From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by smtp.lore.kernel.org (Postfix) with ESMTP id 35694C3DA6E for ; Wed, 3 Jan 2024 13:00:40 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 8B13E6B01F0; Wed, 3 Jan 2024 08:00:39 -0500 (EST) Received: by kanga.kvack.org (Postfix, from userid 40) id 83A8D6B01F2; Wed, 3 Jan 2024 08:00:39 -0500 (EST) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 68D476B01F3; Wed, 3 Jan 2024 08:00:39 -0500 (EST) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0014.hostedemail.com [216.40.44.14]) by kanga.kvack.org (Postfix) with ESMTP id 51C4C6B01F0 for ; Wed, 3 Jan 2024 08:00:39 -0500 (EST) Received: from smtpin11.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay03.hostedemail.com (Postfix) with ESMTP id 570EDA08AB for ; Wed, 3 Jan 2024 13:00:34 +0000 (UTC) X-FDA: 81638008788.11.41358C4 Received: from nautica.notk.org (nautica.notk.org [91.121.71.147]) by imf05.hostedemail.com (Postfix) with ESMTP id 1D65910001F for ; Wed, 3 Jan 2024 13:00:30 +0000 (UTC) Authentication-Results: imf05.hostedemail.com; dkim=pass header.d=codewreck.org header.s=2 header.b=BkgcQhUY; dkim=pass header.d=codewreck.org header.s=2 header.b=4AiBjprU; dmarc=pass (policy=none) header.from=codewreck.org; spf=pass (imf05.hostedemail.com: domain of asmadeus@codewreck.org designates 91.121.71.147 as permitted sender) smtp.mailfrom=asmadeus@codewreck.org ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1704286831; h=from:from:sender:reply-to:subject:subject:date:date: message-id:message-id:to:to:cc:cc:mime-version:mime-version: content-type:content-type:content-transfer-encoding: in-reply-to:in-reply-to:references:references:dkim-signature; bh=M9MVRJ/IOq68RjnYFcSXCjl3WBemBARgVfqAaYYq2bU=; b=nLN/zIinpJAKiTvuXcnAGiOo1GHHTFXKOi7sDqIkL9XbkOfQJD17QM+m+cfM9ZiZ2N06M7 XnVKBOkp7nVs13tnyOPVmsMwdVQPaiRen92ktW+3JZAhc+2dJVmh/J8Llybo3tjwmshoLv g97a1sKtCIKvueju8yv+eu8i4BCaNWc= ARC-Authentication-Results: i=1; imf05.hostedemail.com; dkim=pass header.d=codewreck.org header.s=2 header.b=BkgcQhUY; dkim=pass header.d=codewreck.org header.s=2 header.b=4AiBjprU; dmarc=pass (policy=none) header.from=codewreck.org; spf=pass (imf05.hostedemail.com: domain of asmadeus@codewreck.org designates 91.121.71.147 as permitted sender) smtp.mailfrom=asmadeus@codewreck.org ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1704286831; a=rsa-sha256; cv=none; b=w7y39VjudkF9/P9RxB6ZSJo24N6V7a1qqElRqnqqsSFv6l0Hc4+m47R8nAPuC6Xyty3+jm UgCfPg8CnezJGLKSocZdES7mfDArkoKST8i1SmFnewoC++KstUwN3sLRz8uY4g/UwdUJUi eiHaqElxqDoipn7FvOI/MQZCYFJzftY= Received: by nautica.notk.org (Postfix, from userid 108) id 69177C020; Wed, 3 Jan 2024 14:00:29 +0100 (CET) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=codewreck.org; s=2; t=1704286829; bh=M9MVRJ/IOq68RjnYFcSXCjl3WBemBARgVfqAaYYq2bU=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=BkgcQhUY06Yv7TNWtoFJ3BD8uhuRBFoGY+5r4fJ2pFtWm/MsyOidk3WH3Cbt/BHqe p+Rovs9uu9HY0kk3fuoU+z45KVaZlgLrVhaRYdqfjLDVEYRPf86/FV6mnI2dFqnQc0 CJmvCjN3jVqbir6n+dhLEE2+2tUqVt+ENPGDQgTVAmU07WpC+7YTMgwTN9GMqFWBld K+Fw8SiQzMmKxJDgZ9AunEvmr5ZUbrHOs7ciNQS3iQfXppDADqLdPwcKulUHK2xsrA /Ons7jRB9Yk0hDKwK/eUmkQhtlslwECpqtOhS082N3E6nsg9isZ3IC9emlh6zb4x+T IxlY0TrI8rsnQ== Received: from gaia (localhost [127.0.0.1]) by nautica.notk.org (Postfix) with ESMTPS id C3C5DC009; Wed, 3 Jan 2024 14:00:21 +0100 (CET) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=codewreck.org; s=2; t=1704286828; bh=M9MVRJ/IOq68RjnYFcSXCjl3WBemBARgVfqAaYYq2bU=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=4AiBjprUNXrqU3zwYpywyL3jd+SefRgEQ0LitNyZbaCbkJABDGm+1uVTVbFh7u4Eb U82ANNe4mKUhzMSta94yKyXND+fzKyvpqFdCLn9a7UjnvEjdop7jxDsasrVnP+qxgi wZeEpsE4ZCdUj1/HSviuG7djlA7fGo7ODJQKtNfKXSkkSjFUf2QxuzmjQSeUhJQ58P kKZ2y7CZvt1mOp6hJBUR34+jt64lDp9p2FiUPTlL/yKOslM0yOFoDXfyIdC8kMuUMs 1IIsLCN0iO14LaRP68hQ6P5k46JP32PsH56gy9vyr5FyEDZn2pnuxCIRYku7lF1ywb 6cIfPLs+CCkTw== Received: from localhost (gaia [local]) by gaia (OpenSMTPD) with ESMTPA id f04d2000; Wed, 3 Jan 2024 13:00:18 +0000 (UTC) Date: Wed, 3 Jan 2024 22:00:03 +0900 From: Dominique Martinet To: David Howells Cc: Jeff Layton , Steve French , Matthew Wilcox , Marc Dionne , Paulo Alcantara , Shyam Prasad N , Tom Talpey , Eric Van Hensbergen , Ilya Dryomov , Christian Brauner , linux-cachefs@redhat.com, linux-afs@lists.infradead.org, linux-cifs@vger.kernel.org, linux-nfs@vger.kernel.org, ceph-devel@vger.kernel.org, v9fs@lists.linux.dev, linux-fsdevel@vger.kernel.org, linux-mm@kvack.org, netdev@vger.kernel.org, linux-kernel@vger.kernel.org, Latchesar Ionkov , Christian Schoenebeck Subject: Re: [PATCH v5 40/40] 9p: Use netfslib read/write_iter Message-ID: References: <20231221132400.1601991-1-dhowells@redhat.com> <20231221132400.1601991-41-dhowells@redhat.com> <355430.1704285574@warthog.procyon.org.uk> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <355430.1704285574@warthog.procyon.org.uk> X-Rspamd-Queue-Id: 1D65910001F X-Rspam-User: X-Rspamd-Server: rspam05 X-Stat-Signature: gping36wpri57tx5u3m4jhpzbb83rosq X-HE-Tag: 1704286830-363661 X-HE-Meta: U2FsdGVkX1+xF3KK3i4zEVtEEYWVzDw1f5Wzc2fbtkqQsRoOsN9z3Eq4m5z+ABwR/QeBasnUxOD9uOxF1eInwiBzWRbbRyYtfJ2v1EryHkPefxb3V19FsD2BtW96QKPWkSgpcGvO6YcuG8KC8PV6Bnt7PkM/4rXgGriR2x7grTHd1K8Dx1sMXHlJFBk9PcSCv/6mOcgP+tRssQ2R3Eh1Y6DXAy8qoof/bVsA38Lkl4HF/BsEoDLbhUYsTHt8zu+ABY+MOQSktOxLhCjhqsnVUnzFksBsO0D7h6q3Xx6KYrtRf3G1yTLo++v2JXewvUrpdmA+aYjsNrULx5mvrlaQp4J5aDuxjLfEitFMytWuBuhfxJ5ctbu1WsI3JoswzlKHKrO0/5dAlEY8ZOXuk4oLA3e+6WGv95f2wNpEOZ0uWtwhDy9v1aRPcL5pD/lvOn2Q7OTf6YS+GMhFNHq4v5Q8ai+0u6ZyKituyLOICYwk06+khFGHAyZznivkd0KH0qHJRc3AFMPPdZKlYD+zzn5sRqP6hON7obZ1Fx6sNj7lb/uTQXjwfbwSGuvbtuqh4QhOm5Fpy3HwDg4SG40U93mM3EXdDZgrSGZe2E9MT8lpRfRStxCr+oq/RKZ/ZmweHPhh1lwPTBsudSvFXVZtfA09zbWmhV2xzUs/qwkaKTggVHnytk5a752zT0ZbLmJjK9bK6TpYpCmyxhSZPQ9V/OZEG9AVWMQ7Lod/ST7PXewG/LczIE6JRMkev2nXTG9dJKrjXKdUbZqR22lQJFKr9DllYnBid7GeI5Ea3UxtXlH4y55/KRmfAvoPfv6+mtrn9+CHIYkJCKPKLZ8mvjo1vHiEJvuD+lT6paGbYuDNEp/Rj89ZqWkkSeNQ4qjQ+6qIyQugPz2xh+b9fEp6J5dBiAwFlMksECsDwPwYALCxRm598v1XjYl+BlfMf6q1/hcQ0npymf8OkNSAeLyodAHvfik LKofdJpa OJmHb3FhyUNDNqIjJ27jBIg5TJVkJz7JE+Dk5yqVkdoOBsUVJ8oozaQppAFoT1RzczyXLBrffFWThiXnePS3OVJ0GQsA/SkItynf+x0Xyh4WBPTlt7WYH10iozIOCuaElndBDX/ZBwoy7SHX6d5CkLkK9Cv+qu2Qp77R8Idapv8WQYlYR6vNpWTgSNrBz15kYAmQbWil05AE3odYOGQmVh3RSQthIGapGAZrP X-Bogosity: Ham, tests=bogofilter, spamicity=0.000000, version=1.2.4 Sender: owner-linux-mm@kvack.org Precedence: bulk X-Loop: owner-majordomo@kvack.org List-ID: List-Subscribe: List-Unsubscribe: David Howells wrote on Wed, Jan 03, 2024 at 12:39:34PM +0000: > > p9_client_write return value should always be subreq->len, but I believe > > we should use it unless err is set. > > (It's also possible for partial writes to happen, e.g. p9_client_write > > looped a few times and then failed, at which point the size returned > > would be the amount that actually got through -- we probably should do > > something with that?) > > How about something like: > > - int err; > + int err, len; > > trace_netfs_sreq(subreq, netfs_sreq_trace_submit); > - p9_client_write(fid, subreq->start, &subreq->io_iter, &err); > - netfs_write_subrequest_terminated(subreq, err < 0 ? err : subreq->len, > - false); > + len = p9_client_write(fid, subreq->start, &subreq->io_iter, &err); > + netfs_write_subrequest_terminated(subreq, len ?: err, false); I think that'll be fine; plain write() syscall works like this when an error happens after some data has been flushed, and I assume there'll be some retry if this happpened on something like a flush dirty and it got a partial write reported? > > > + if (file) { > > > + fid = file->private_data; > > > + BUG_ON(!fid); > > > > This probably should be WARN + return EINVAL like find by inode? > > It's certainly a huge problem, but we should avoid BUG if possible... > > Sure. The BUG_ON() was already there, but I can turn it into a WARN+error. Thanks. > > nit: not sure what's cleaner? > > Since there's a message that makes for a bit awkward if... > > > > if (WARN_ONCE(!fid, "folio expected an open fid inode->i_private=%p\n", > > rreq->inode->i_private)) > > return -EINVAL; > > > > (as a side note, I'm not sure what to make of this i_private pointer > > here, but if that'll help you figure something out sure..) > > Um. 9p is using i_private. But perhaps i_ino would be a better choice: > > if (file) { > fid = file->private_data; > if (!fid) > goto no_fid; > p9_fid_get(fid); > } else { > fid = v9fs_fid_find_inode(rreq->inode, writing, INVALID_UID, true); > if (!fid) > goto no_fid; > } > > ... > > no_fid: > WARN_ONCE(1, "folio expected an open fid inode->i_ino=%lx\n", > rreq->inode->i_ino); > return -EINVAL; Might be useful to track down if this came frm a file without private data or lookup failing, but given this was a bug I guess we can deal with that when that happens -- ack. > > This is as follow on your netfs-lib branch: > > - WARN_ON(rreq->origin == NETFS_READ_FOR_WRITE && > > - !(fid->mode & P9_ORDWR)); > > - > > - p9_fid_get(fid); > > + WARN_ON(rreq->origin == NETFS_READ_FOR_WRITE && !(fid->mode & P9_ORDWR)); > > > > So the WARN_ON has been reverted back with only indentation changed; > > I guess there were patterns that were writing despite the fid not having > > been open as RDWR? > > Do you still have details about these? > > The condition in the WARN_ON() here got changed. It was: > > WARN_ON(writing && ... > > at one point, but that caused a bunch of incorrect warning to appear because > only NETFS_READ_FOR_WRITE requires read-access as well as write-access. All > the others: > > bool writing = (rreq->origin == NETFS_READ_FOR_WRITE || > rreq->origin == NETFS_WRITEBACK || > rreq->origin == NETFS_WRITETHROUGH || > rreq->origin == NETFS_LAUNDER_WRITE || > rreq->origin == NETFS_UNBUFFERED_WRITE || > rreq->origin == NETFS_DIO_WRITE); > > only require write-access. Thanks for clarifying > > If a file has been open without the write bit it might not go through, > > and it's incredibly difficult to get such users back to userspace in > > async cases (e.g. mmap flushes), so would like to understand that. > > The VFS/VM should prevent writing to files that aren't open O_WRONLY or > O_RDWR, so I don't think we should be called in otherwise. Historically this check was more about finding a fid that wasn't opened properly than the VFS doing something weird (e.g. by calling mprotect after mmap and us missing that -- would need to check if that works actually...) > > > + return netfs_page_mkwrite(vmf, NULL); > > > > (I guess there's no helper that could be used directly in .page_mkwrite > > op?) > > I could provide a helper that just supplies NULL as the second argument. I > think only 9p will use it, but that's fine. If we're the only user I guess we shouldn't bother with it at this point, we can come back to it if this ever becomes common. -- Dominique Martinet | Asmadeus