On Wed, 26 Jul 2023, Jeff Layton wrote:
> On Tue, 2023-07-25 at 17:41 +0000, Trond Myklebust wrote:
> > On Tue, 2023-07-25 at 12:24 -0400, Scott Mayhew wrote:
> > > On Tue, 25 Jul 2023, Trond Myklebust wrote:
> > >
> > > > On Tue, 2023-07-25 at 11:08 -0400, Scott Mayhew wrote:
> > > > > Once a folio's private data has been cleared, it's possible for
> > > > > another
> > > > > process to clear the folio->mapping (e.g. via
> > > > > invalidate_complete_folio2
> > > > > or evict_mapping_folio), so it wouldn't be safe to call
> > > > > nfs_page_to_inode() after that.
> > > > >
> > > > > Fixes: 0c493b5cf16e ("NFS: Convert buffered writes to use
> > > > > folios")
> > > > > Signed-off-by: Scott Mayhew <smayhew@xxxxxxxxxx>
> > > > > ---
> > > > > fs/nfs/write.c | 4 +++-
> > > > > 1 file changed, 3 insertions(+), 1 deletion(-)
> > > > >
> > > > > diff --git a/fs/nfs/write.c b/fs/nfs/write.c
> > > > > index f4cca8f00c0c..489c3f9dae23 100644
> > > > > --- a/fs/nfs/write.c
> > > > > +++ b/fs/nfs/write.c
> > > > > @@ -785,6 +785,8 @@ static void nfs_inode_add_request(struct
> > > > > nfs_page
> > > > > *req)
> > > > > */
> > > > > static void nfs_inode_remove_request(struct nfs_page *req)
> > > > > {
> > > > > + struct nfs_inode *nfsi = NFS_I(nfs_page_to_inode(req));
> > > > > +
> > > > > if (nfs_page_group_sync_on_bit(req, PG_REMOVE)) {
> > > > > struct folio *folio = nfs_page_to_folio(req-
> > > > > > wb_head);
> > > > > struct address_space *mapping =
> > > > > folio_file_mapping(folio);
> > > > > @@ -800,7 +802,7 @@ static void nfs_inode_remove_request(struct
> > > > > nfs_page *req)
> > > > >
> > > > > if (test_and_clear_bit(PG_INODE_REF, &req->wb_flags)) {
> > > > > nfs_release_request(req);
> > > > > - atomic_long_dec(&NFS_I(nfs_page_to_inode(req))-
> > > > > > nrequests);
> > > > > + atomic_long_dec(&nfsi->nrequests);
> > > >
> > > > Why not just invert the order of the atomic_long_dec() and the
> > > > nfs_release_request()? That way you are also ensuring that the
> > > > inode is
> > > > still pinned in memory by the open context.
> > >
> > > I'm not following. How does inverting the order prevent the
> > > folio->mapping from getting clobbered?
> > >
> >
> > The open/lock context is refcounted by the nfs_page until the latter is
> > released. That's why the inode is guaranteed to remain around at least
> > until the call to nfs_release_request().
> >
>
> The problem is not that the inode is going away, but rather that we
> can't guarantee that the page is still part of the mapping at this
> point, and so we can't safely dereference page->mapping there. I do see
> that nfs_release_request releases a reference to the page, but I don't
> think that's sufficient to ensure that it remains part of the mapping.
>
> AFAICT, once we clear page->private, the page is subject to be removed
> from the mapping. So, I *think* it's safe to just move the call to
> nfs_page_to_inode prior to the call to nfs_page_group_sync_on_bit.
Yeah, the inode hasn't gone away. I can pick the nfs_commit_data
address off the stack in nfs_commit_release_pages:
crash> nfs_commit_data.inode c0000006774cae00
inode = 0xc00000006c1b05f8,
The nfs_inode is still allocated:
crash> kmem 0xc00000006c1b05f8
CACHE OBJSIZE ALLOCATED TOTAL SLABS SSIZE NAME
c000000030332600 1088 128 5959 101 64k nfs_inode_cache
SLAB MEMORY NODE TOTAL ALLOCATED FREE
c00c0000001b06c0 c00000006c1b0000 0 59 1 58
FREE / [ALLOCATED]
[c00000006c1b0448]
PAGE PHYSICAL MAPPING INDEX CNT FLAGS
c00c0000001b06c0 6c1b0000 c000000030332600 c00000006c1b4480 1 23ffff800000200 slab
The vfs_inode:
crash> px &((struct nfs_inode *)0xc00000006c1b0448)->vfs_inode
$7 = (struct inode *) 0xc00000006c1b05f8
Matches the inodes open by both nfs_flock programs from the test:
crash> foreach nfs_flock files
PID: 4006780 TASK: c00000009d436600 CPU: 43 COMMAND: "nfs_flock"
ROOT: / CWD: /tmp/ltp-aFr4AJt3R9/LTP_nfslock01.9hyHNgoKqq/3/0/
FD FILE DENTRY INODE TYPE PATH
0 c000000196e9a000 c000000004090840 c00000000ae13bf0 CHR /dev/null
1 c0000000bfd1ff00 c0000000963e0e40 c00000006c573900 REG /opt/ltp/output/nfslock01.sh_20230610112802
2 c0000000bfd1ff00 c0000000963e0e40 c00000006c573900 REG /opt/ltp/output/nfslock01.sh_20230610112802
3 c000000196e97700 c000000419ccb040 c00000006c1b05f8 REG /tmp/ltp-aFr4AJt3R9/LTP_nfslock01.9hyHNgoKqq/3/0/flock_idata
^^^^^^^^^^^^^^^^
PID: 4006781 TASK: c00000009d42d500 CPU: 42 COMMAND: "nfs_flock"
ROOT: / CWD: /tmp/ltp-aFr4AJt3R9/LTP_nfslock01.9hyHNgoKqq/3/0/
FD FILE DENTRY INODE TYPE PATH
0 c0000000f0812200 c000000004090840 c00000000ae13bf0 CHR /dev/null
1 c0000000bfd1ff00 c0000000963e0e40 c00000006c573900 REG /opt/ltp/output/nfslock01.sh_20230610112802
2 c0000000bfd1ff00 c0000000963e0e40 c00000006c573900 REG /opt/ltp/output/nfslock01.sh_20230610112802
3 c0000000f0813c00 c000000419ccb040 c00000006c1b05f8 REG /tmp/ltp-aFr4AJt3R9/LTP_nfslock01.9hyHNgoKqq/3/0/flock_idata
^^^^^^^^^^^^^^^^
The file->f_mapping for both struct files matches the inode->i_data:
crash> file.f_mapping c000000196e97700
f_mapping = 0xc00000006c1b0770,
crash> file.f_mapping c0000000f0813c00
f_mapping = 0xc00000006c1b0770,
crash> px &((struct inode *)0xc00000006c1b05f8)->i_data
$8 = (struct address_space *) 0xc00000006c1b0770
and if I look at one of those nfs_flock tasks, the folio
passed in to nfs_read_folio has the same mapping:
crash> bt 4006781
PID: 4006781 TASK: c00000009d42d500 CPU: 42 COMMAND: "nfs_flock"
#0 [c000000177053710] __schedule at c000000000f61d9c
#1 [c0000001770537d0] schedule at c000000000f621f4
#2 [c000000177053840] io_schedule at c000000000f62354
#3 [c000000177053870] folio_wait_bit_common at c00000000042dc60
#4 [c000000177053970] nfs_read_folio at c0080000050108a8 [nfs]
#5 [c000000177053a60] nfs_write_begin at c008000004fff06c [nfs]
#6 [c000000177053b10] generic_perform_write at c00000000042b044
#7 [c000000177053bc0] nfs_file_write at c008000004ffda08 [nfs]
#8 [c000000177053c60] new_sync_write at c00000000057fdd8
#9 [c000000177053d10] vfs_write at c000000000582fd4
#10 [c000000177053d60] ksys_write at c0000000005833a4
#11 [c000000177053db0] system_call_exception at c00000000002f434
#12 [c000000177053e10] system_call_vectored_common at c00000000000bfe8
crash> folio.mapping c00c000000564400
mapping = 0xc00000006c1b0770,
It's just that if we go back to the nfs_page being released by our panic
task, the folio->mapping has been cleared, so we panic when we try to go
folio->mapping->host.
crash> nfs_page c00000016fb2a600
struct nfs_page {
wb_list = {
next = 0xc00000016fb2a600,
prev = 0xc00000016fb2a600
},
{
wb_page = 0xc00c000001d49580,
wb_folio = 0xc00c000001d49580
},
wb_lock_context = 0xc00000010518b2c0,
wb_index = 0x1,
wb_offset = 0x6940,
wb_pgbase = 0x6940,
wb_bytes = 0x40,
wb_kref = {
refcount = {
refs = {
counter = 0x1
}
}
},
wb_flags = 0x5,
wb_verf = {
data = "\214\205_d\214\210W\036"
},
wb_this_page = 0xc00000016fb2a600,
wb_head = 0xc00000016fb2a600,
wb_nio = 0x0
}
crash> folio.mapping 0xc00c000001d49580
mapping = 0x0,
-Scott
> --
> Jeff Layton <jlayton@xxxxxxxxxx>
>
