On Dienstag, 14. Juni 2022 05:41:40 CEST Dominique Martinet wrote:
Dominique Martinet wrote on Tue, Jun 14, 2022 at 12:38:02PM +0900:
cached operations sometimes need to do invalid operations (e.g. read on a write only file) Historic fscache had added a "writeback fid" for this, but the conversion to new fscache somehow lost usage of it: use the writeback fid instead of normal one.
Note that the way this works (writeback fid being linked to inode) means we might use overprivileged fid for some operations, e.g. write as root when we shouldn't. Ideally we should keep both fids handy, and only use the writeback fid when really required e.g. reads to a write-only file to fill in the page cache (read-modify-write); but this is the situation we've always had and this commit only fixes an issue we've had for too long.
Fixes: eb497943fa21 ("9p: Convert to using the netfs helper lib to do reads and caching") Cc: stable@vger.kernel.org Cc: David Howells dhowells@redhat.com Reported-By: Christian Schoenebeck linux_oss@crudebyte.com Signed-off-by: Dominique Martinet asmadeus@codewreck.org
Ok so finally had time to look at this, and it's not a lot so this is the most straight forward way to do: just reverting to how the old fscache worked.
This appears to work from quick testing, Chiristian could you test it?
I think the warnings you added in p9_client_read/write that check fid->mode might a lot of sense, if you care to resend it as WARN_ON((fid->mode & ACCMODE) == O_xyz); instead I'll queue that for 5.20
@Stable people, I've checked it applies to 5.17 and 5.18 so should be good to grab once I submit it for inclusion (that commit was included in 5.16, which is no longer stable)
fs/9p/vfs_addr.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/fs/9p/vfs_addr.c b/fs/9p/vfs_addr.c index 7382c5227e94..262968d02f55 100644 --- a/fs/9p/vfs_addr.c +++ b/fs/9p/vfs_addr.c @@ -58,7 +58,11 @@ static void v9fs_issue_read(struct netfs_io_subrequest *subreq)> */ static int v9fs_init_request(struct netfs_io_request *rreq, struct file *file) {
- struct p9_fid *fid = file->private_data;
- struct inode *inode = file_inode(file);
- struct v9fs_inode *v9inode = V9FS_I(inode);
- struct p9_fid *fid = v9inode->writeback_fid;
Sorry for mails back-to-back (grmbl I hate git commit --amend not warning I only have unstaged changes), this is missing the following here:
I think git does actually. It shows you staged and unstaged changes as comment below the commit log text inside the editor. Not as a big fat warning, but the info is there.
- /* If there is no writeback fid this file only ever has had
* read-only opens, so we can use file's fid which should* always be set instead */- if (!fid)
fid = file->private_data;Christian, you can find it here to test: https://github.com/martinetd/linux/commit/a6e033c41cc9f0ec105f5d208b0a820118 e2bda8
BUG_ON(!fid);
p9_fid_get(fid); rreq->netfs_priv = fid;
It definitely goes into the right direction, but I think it's going a bit too far by using writeback_fid also in cases where it is not necessary and wasn't used before in the past.
What about something like this in v9fs_init_request() (yet untested):
/* writeback_fid is always opened O_RDWR (instead of just O_WRONLY) * explicitly for this case: partial write backs that require a read * prior to actual write and therefore requires a fid with read * capability. */ if (rreq->origin == NETFS_READ_FOR_WRITE) fid = v9inode->writeback_fid;
If desired, this could be further constrained later on like:
if (rreq->origin == NETFS_READ_FOR_WRITE && (fid->mode & O_ACCMODE) == O_WRONLY) { fid = v9inode->writeback_fid; }
I will definitely give these options some test spins here, a short feedback ahead would be appreciated though.
Thanks Dominique!
Best regards, Christian Schoenebeck