fs/9p/vfs_addr.c | 13 +++++++++++++ 1 file changed, 13 insertions(+)
cached operations sometimes need to do invalid operations (e.g. read
on a write only file)
Historic fscache had added a "writeback fid", a special handle opened
RW as root, for this. The conversion to new fscache missed that bit.
This commit reinstates a slightly lesser variant of the original code
that uses the writeback fid for partial pages backfills if the regular
user fid had been open as WRONLY, and thus would lack read permissions.
Link: https://lkml.kernel.org/r/20220614033802.1606738-1-asmadeus@codewreck.org
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>
Reviewed-by: Christian Schoenebeck <linux_oss@crudebyte.com>
Tested-by: Christian Schoenebeck <linux_oss@crudebyte.com>
Signed-off-by: Dominique Martinet <asmadeus@codewreck.org>
---
v3: use the least permissive version of the patch that only uses
writeback fid when really required
If no problem shows up by then I'll post this patch around Wed 23 (next
week) with the other stable fixes.
fs/9p/vfs_addr.c | 13 +++++++++++++
1 file changed, 13 insertions(+)
diff --git a/fs/9p/vfs_addr.c b/fs/9p/vfs_addr.c
index a8f512b44a85..d0833fa69faf 100644
--- a/fs/9p/vfs_addr.c
+++ b/fs/9p/vfs_addr.c
@@ -58,8 +58,21 @@ static void v9fs_issue_read(struct netfs_io_subrequest *subreq)
*/
static int v9fs_init_request(struct netfs_io_request *rreq, struct file *file)
{
+ struct inode *inode = file_inode(file);
+ struct v9fs_inode *v9inode = V9FS_I(inode);
struct p9_fid *fid = file->private_data;
+ BUG_ON(!fid);
+
+ /* we might need to read from a fid that was opened write-only
+ * for read-modify-write of page cache, use the writeback fid
+ * for that */
+ if (rreq->origin == NETFS_READ_FOR_WRITE &&
+ (fid->mode & O_ACCMODE) == O_WRONLY) {
+ fid = v9inode->writeback_fid;
+ BUG_ON(!fid);
+ }
+
refcount_inc(&fid->count);
rreq->netfs_priv = fid;
return 0;
--
2.35.1On Donnerstag, 16. Juni 2022 23:10:25 CEST Dominique Martinet wrote:
> cached operations sometimes need to do invalid operations (e.g. read
> on a write only file)
> Historic fscache had added a "writeback fid", a special handle opened
> RW as root, for this. The conversion to new fscache missed that bit.
>
> This commit reinstates a slightly lesser variant of the original code
> that uses the writeback fid for partial pages backfills if the regular
> user fid had been open as WRONLY, and thus would lack read permissions.
>
> Link:
> https://lkml.kernel.org/r/20220614033802.1606738-1-asmadeus@codewreck.org
> 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>
> Reviewed-by: Christian Schoenebeck <linux_oss@crudebyte.com>
> Tested-by: Christian Schoenebeck <linux_oss@crudebyte.com>
> Signed-off-by: Dominique Martinet <asmadeus@codewreck.org>
> ---
> v3: use the least permissive version of the patch that only uses
> writeback fid when really required
>
> If no problem shows up by then I'll post this patch around Wed 23 (next
> week) with the other stable fixes.
>
> fs/9p/vfs_addr.c | 13 +++++++++++++
> 1 file changed, 13 insertions(+)
>
> diff --git a/fs/9p/vfs_addr.c b/fs/9p/vfs_addr.c
> index a8f512b44a85..d0833fa69faf 100644
> --- a/fs/9p/vfs_addr.c
> +++ b/fs/9p/vfs_addr.c
> @@ -58,8 +58,21 @@ static void v9fs_issue_read(struct netfs_io_subrequest
> *subreq) */
> static int v9fs_init_request(struct netfs_io_request *rreq, struct file
> *file) {
> + struct inode *inode = file_inode(file);
> + struct v9fs_inode *v9inode = V9FS_I(inode);
> struct p9_fid *fid = file->private_data;
>
> + BUG_ON(!fid);
> +
> + /* we might need to read from a fid that was opened write-only
> + * for read-modify-write of page cache, use the writeback fid
> + * for that */
> + if (rreq->origin == NETFS_READ_FOR_WRITE &&
> + (fid->mode & O_ACCMODE) == O_WRONLY) {
> + fid = v9inode->writeback_fid;
> + BUG_ON(!fid);
> + }
> +
> refcount_inc(&fid->count);
> rreq->netfs_priv = fid;
> return 0;
Some more tests this weekend; all looks fine. It appears that this also fixed
the performance degradation that I reported early in this thread. Again,
benchmarks compiling a bunch of sources:
Case Linux kernel version msize cache duration (average)
A) EBADF fix only [1] 512000 loose 31m 14s
B) EBADF fix only [1] 512000 mmap 44m 1s
C) EBADF fix + clunk fixes [2] 512000 loose 29m 32s
D) EBADF fix + clunk fixes [2] 512000 mmap 44m 0s
E) 5.10.84 512000 loose 35m 5s
F) 5.10.84 512000 mmap 65m 5s
[1] 5.19.0-rc2 + EBADF fix v3 patch (alone):
https://lore.kernel.org/lkml/20220616211025.1790171-1-asmadeus@codewreck.org/
[2] 5.19.0-rc2 + EBADF fix v3 patch + clunk fix patches, a.k.a. 9p-next:
https://github.com/martinetd/linux/commit/b0017602fdf6bd3f344dd49eaee8b6ffeed6dbac
Conclusion: all thumbs in my possession pointing upwards. :)
Thanks Dominique!
Best regards,
Christian Schoenebeck
Christian Schoenebeck wrote on Mon, Jun 20, 2022 at 02:47:24PM +0200: > Some more tests this weekend; all looks fine. It appears that this also fixed > the performance degradation that I reported early in this thread. wow, I wouldn't have expected the EBADF fix patch to have any impact on performance. Maybe the build just behaved differently enough to take more time with the errors? > Again, benchmarks compiling a bunch of sources: > > Case Linux kernel version msize cache duration (average) > > A) EBADF fix only [1] 512000 loose 31m 14s > B) EBADF fix only [1] 512000 mmap 44m 1s > C) EBADF fix + clunk fixes [2] 512000 loose 29m 32s > D) EBADF fix + clunk fixes [2] 512000 mmap 44m 0s > E) 5.10.84 512000 loose 35m 5s > F) 5.10.84 512000 mmap 65m 5s > > [1] 5.19.0-rc2 + EBADF fix v3 patch (alone): > https://lore.kernel.org/lkml/20220616211025.1790171-1-asmadeus@codewreck.org/ > > [2] 5.19.0-rc2 + EBADF fix v3 patch + clunk fix patches, a.k.a. 9p-next: > https://github.com/martinetd/linux/commit/b0017602fdf6bd3f344dd49eaee8b6ffeed6dbac > > Conclusion: all thumbs in my possession pointing upwards. :) > > Thanks Dominique! Great news, thanks for the tests! :) -- Dominique
On Montag, 20. Juni 2022 22:34:38 CEST Dominique Martinet wrote: > Christian Schoenebeck wrote on Mon, Jun 20, 2022 at 02:47:24PM +0200: > > Some more tests this weekend; all looks fine. It appears that this also > > fixed the performance degradation that I reported early in this thread. > > wow, I wouldn't have expected the EBADF fix patch to have any impact on > performance. Maybe the build just behaved differently enough to take > more time with the errors? Maybe. It could also be less overhead using writeback_fid vs. dedicated fid, i.e. no walking and fid cloning required when just using the writeback_fid which is already there (reduced latency). Probably also overall reduced total amount of fids might have some (smaller) impact, as on QEMU 9p server side we still have a simple linked list for fids which is iterated on each fid lookup. A proc-like interface for statistics (e.g. max. amount of fids) would be useful. But honestly, all these things still don't really explain to me such a difference from performance PoV in regards to this patch, as the particular case handled by this patch does not appear to happen often. Anyway, my plan is to identify performance bottlenecks in general more analytically this year. Now that we have macOS support for 9p in QEMU, I'll probably use Xcode's "Instruments" tool which really has a great way to graphically investigate complex performance aspects in a very intuitive and customizable way, which goes beyond standard profiling. Then I can hunt down performance issues by weight. Best regards, Christian Schoenebeck
© 2016 - 2026 Red Hat, Inc.