lkml.org 
[lkml]   [2022]   [Jun]   [12]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
Patch in this message
/
Date
From
SubjectRe: 9p EBADF with cache enabled (Was: 9p fs-cache tests/benchmark (was: 9p fscache Duplicate cookie detected))

Sorry, I had planned on working on this today but the other patchset
ended up taking all my time... I think I'm bad at priorities, this
is definitely important...

David, I think with the latest comments we made it should be relatively
straightforward to make netfs use the writeback fid? Could you find some
time to have a look? It should be trivial to reproduce, I gave these
commands a few mails ago (needs to run as a regular user, on a fscache mount)
---
$ dd if=/dev/zero of=test bs=1M count=1
$ chmod 200 test
# drop cache or remount
$ dd if=/dev/urandom of=test bs=102 seek=2 count=1 conv=notrunc
dd: error writing 'test': Bad file descriptor
---

Otherwise I'll try to make some more 9p time again, but it's getting
more and more difficult for me...

Christian Schoenebeck wrote on Fri, Jun 03, 2022 at 06:46:04PM +0200:
> I had another time slice on this issue today. As Dominique pointed out before,
> the writeback_fid was and still is opened with O_RDWR [fs/9p/fid.c]:
>
> struct p9_fid *v9fs_writeback_fid(struct dentry *dentry)
> {
> int err;
> struct p9_fid *fid, *ofid;
>
> ofid = v9fs_fid_lookup_with_uid(dentry, GLOBAL_ROOT_UID, 0);
> fid = clone_fid(ofid);
> if (IS_ERR(fid))
> goto error_out;
> p9_client_clunk(ofid);
> /*
> * writeback fid will only be used to write back the
> * dirty pages. We always request for the open fid in read-write
> * mode so that a partial page write which result in page
> * read can work.
> */
> err = p9_client_open(fid, O_RDWR);
> if (err < 0) {
> p9_client_clunk(fid);
> fid = ERR_PTR(err);
> goto error_out;
> }
> error_out:
> return fid;
> }
>
> The problem rather seems to be that the new netfs code does not use the
> writeback_fid when doing an implied read before the actual partial writeback.
>
> As I showed in my previous email, the old pre-netfs kernel versions also did a
> read before partial writebacks, but apparently used the special writeback_fid
> for that.

This looks good! Thanks for keeping it up.

>
> I added some trap code to recent netfs kernel version:
>
> diff --git a/net/9p/client.c b/net/9p/client.c
> index 8bba0d9cf975..11ff1ee2130e 100644
> --- a/net/9p/client.c
> +++ b/net/9p/client.c
> @@ -1549,12 +1549,21 @@ int p9_client_unlinkat(struct p9_fid *dfid, const char *name, int flags)
> }
> EXPORT_SYMBOL(p9_client_unlinkat);
>
> +void p9_bug(void) {
> + BUG_ON(true);
> +}
> +EXPORT_SYMBOL(p9_bug);
> +
> int
> p9_client_read(struct p9_fid *fid, u64 offset, struct iov_iter *to, int *err)
> {
> int total = 0;
> *err = 0;
>
> + if ((fid->mode & O_ACCMODE) == O_WRONLY) {
> + p9_bug();
> + }
> +
> while (iov_iter_count(to)) {
> int count;
>
> @@ -1648,6 +1657,10 @@ p9_client_write(struct p9_fid *fid, u64 offset, struct iov_iter *from, int *err)
> p9_debug(P9_DEBUG_9P, ">>> TWRITE fid %d offset %llu count %zd\n",
> fid->fid, offset, iov_iter_count(from));
>
> + if ((fid->mode & O_ACCMODE) == O_RDONLY) {
> + p9_bug();
> + }
> +
> while (iov_iter_count(from)) {
> int count = iov_iter_count(from);
> int rsize = fid->iounit;
>
> Which triggers the trap in p9_client_read() with cache=loose. Here is the
> backtrace [based on d615b5416f8a1afeb82d13b238f8152c572d59c0]:
>
> [ 139.365314] p9_client_read (net/9p/client.c:1553 net/9p/client.c:1564) 9pnet
> [ 139.148806] v9fs_issue_read (fs/9p/vfs_addr.c:45) 9p
> [ 139.149268] netfs_begin_read (fs/netfs/io.c:91 fs/netfs/io.c:579 fs/netfs/io.c:625) netfs
> [ 139.149725] ? xas_load (lib/xarray.c:211 lib/xarray.c:242)
> [ 139.150057] ? xa_load (lib/xarray.c:1469)
> [ 139.150398] netfs_write_begin (fs/netfs/buffered_read.c:407) netfs
> [ 139.150883] v9fs_write_begin (fs/9p/vfs_addr.c:279 (discriminator 2)) 9p
> [ 139.151293] generic_perform_write (mm/filemap.c:3789)
> [ 139.151721] ? generic_update_time (fs/inode.c:1858)
> [ 139.152112] ? file_update_time (fs/inode.c:2089)
> [ 139.152504] __generic_file_write_iter (mm/filemap.c:3916)
> [ 139.152943] generic_file_write_iter (./include/linux/fs.h:753 mm/filemap.c:3948)
> [ 139.153348] new_sync_write (fs/read_write.c:505 (discriminator 1))
> [ 139.153754] vfs_write (fs/read_write.c:591)
> [ 139.154090] ksys_write (fs/read_write.c:644)
> [ 139.154417] do_syscall_64 (arch/x86/entry/common.c:50 arch/x86/entry/common.c:80)
> [ 139.154776] entry_SYSCALL_64_after_hwframe (arch/x86/entry/entry_64.S:115)
>
> I still had not time to read the netfs code part yet, but I assume netfs falls
> back to a generic 9p read on the O_WRONLY opened fid here, instead of using
> the special O_RDWR opened 'writeback_fid'.
>
> Is there already some info available in the netfs API that the read is
> actually part of a writeback task, so that we could force on 9p driver level
> to use the special writeback_fid for the read in this case instead?

--
Dominique

\
 
 \ /
  Last update: 2022-06-12 12:03    [W:0.095 / U:0.200 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site