Re: parts of pages on NFS being replaced by swaths of NULs

[Jeff Layton <jlayton@kernel.org>]

On Sat, 2025-08-16 at 07:51 -0700, Trond Myklebust wrote:
> On Sat, 2025-08-16 at 09:01 -0400, Jeff Layton wrote:
> > 
> > I finally caught something concrete today. I had the attached
> > bpftrace
> > script running while running the reproducer on a dozen or so
> > machines,
> > and it detected a hole in some data being written:
> > 
> > -------------8<---------------
> > Attached 2 probes
> > Missing nfs_page: ino=10122173116 idx=2 flags=0x15ffff0000000029
> > Hole: ino=10122173116 idx=3 off=10026 size=2262
> > Prev folio: idx=2 flags=0x15ffff0000000028 pgbase=0 bytes=4096 req=0
> > prevreq=0xffff8955b2f55980
> > -------------8<---------------
> > 
> > What this tells us is that the page at idx=2 got submitted to
> > nfs_do_writepage() (so it was marked dirty in the pagecache), but
> > when
> > it got there, folio->private was NULL and it was ignored.
> > 
> > The kernel in this case is based on v6.9, so it's (just) pre-large-
> > folio support. It has a fair number of NFS patches, but not much to
> > this portion of the code. Most of them are are containerization
> > fixes.
> > 
> > I'm looking askance at nfs_inode_remove_request(). It does this:
> > 
> >         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->mapping;
> > 
> >                 spin_lock(&mapping->i_private_lock);
> >                 if (likely(folio)) {
> >                         folio->private = NULL;
> >                         folio_clear_private(folio);
> >                         clear_bit(PG_MAPPED, &req->wb_head-
> > > wb_flags);
> >                 }
> >                 spin_unlock(&mapping->i_private_lock);
> >         }
> > 
> > If nfs_page_group_sync_on_bit() returns true, then the nfs_page gets
> > detached from the folio. Meanwhile, if a new write request comes in
> > just after that, nfs_lock_and_join_requests() will call
> > nfs_cancel_remove_inode() to try to "cancel" PG_REMOVE:
> > 
> > static int
> > nfs_cancel_remove_inode(struct nfs_page *req, struct inode *inode)
> > {
> >         int ret;
> > 
> >         if (!test_bit(PG_REMOVE, &req->wb_flags))
> >                 return 0;
> >         ret = nfs_page_group_lock(req);
> >         if (ret)
> >                 return ret;
> >         if (test_and_clear_bit(PG_REMOVE, &req->wb_flags))
> >                 nfs_page_set_inode_ref(req, inode);
> >         nfs_page_group_unlock(req);                          
> >         return 0;                                    
> > }                     
> > 
> > ...but that does not reattach the nfs_page to the folio. Should it?
> >                         
> 
> That's not sufficient AFAICS. Does the following patch work?
> 

The patch looks good to me and it seems like it should close the race.
I'll have to look at backporting it to v6.9, and we'll have to build
images, etc. Hopefully we will have some results in a week or so. I'll
keep you posted!

Thanks,

> 8<------------------------------------------------------------
> From fc9690dda01f001c6cd11665701394da8ebba1ab Mon Sep 17 00:00:00 2001
> Message-ID: <fc9690dda01f001c6cd11665701394da8ebba1ab.1755355810.git.trond.myklebust@hammerspace.com>
> From: Trond Myklebust <trond.myklebust@hammerspace.com>
> Date: Sat, 16 Aug 2025 07:25:20 -0700
> Subject: [PATCH] NFS: Fix a race when updating an existing write
> 
> After nfs_lock_and_join_requests() tests for whether the request is
> still attached to the mapping, nothing prevents a call to
> nfs_inode_remove_request() from succeeding until we actually lock the
> page group.
> The reason is that whoever called nfs_inode_remove_request() doesn't
> necessarily have a lock on the page group head.
> 
> So in order to avoid races, let's take the page group lock earlier in
> nfs_lock_and_join_requests(), and hold it across the removal of the
> request in nfs_inode_remove_request().
> 
> Reported-by: Jeff Layton <jlayton@kernel.org>
> Fixes: c3f2235782c3 ("nfs: fold nfs_folio_find_and_lock_request into nfs_lock_and_join_requests")
> Signed-off-by: Trond Myklebust <trond.myklebust@hammerspace.com>
> ---
>  fs/nfs/pagelist.c        |  9 +++++----
>  fs/nfs/write.c           | 29 ++++++++++-------------------
>  include/linux/nfs_page.h |  1 +
>  3 files changed, 16 insertions(+), 23 deletions(-)
> 
> diff --git a/fs/nfs/pagelist.c b/fs/nfs/pagelist.c
> index 11968dcb7243..6e69ce43a13f 100644
> --- a/fs/nfs/pagelist.c
> +++ b/fs/nfs/pagelist.c
> @@ -253,13 +253,14 @@ nfs_page_group_unlock(struct nfs_page *req)
>  	nfs_page_clear_headlock(req);
>  }
>  
> -/*
> - * nfs_page_group_sync_on_bit_locked
> +/**
> + * nfs_page_group_sync_on_bit_locked - Test if all requests have @bit set
> + * @req: request in page group
> + * @bit: PG_* bit that is used to sync page group
>   *
>   * must be called with page group lock held
>   */
> -static bool
> -nfs_page_group_sync_on_bit_locked(struct nfs_page *req, unsigned int bit)
> +bool nfs_page_group_sync_on_bit_locked(struct nfs_page *req, unsigned int bit)
>  {
>  	struct nfs_page *head = req->wb_head;
>  	struct nfs_page *tmp;
> diff --git a/fs/nfs/write.c b/fs/nfs/write.c
> index fa5c41d0989a..8b7c04737967 100644
> --- a/fs/nfs/write.c
> +++ b/fs/nfs/write.c
> @@ -153,20 +153,10 @@ nfs_page_set_inode_ref(struct nfs_page *req, struct inode *inode)
>  	}
>  }
>  
> -static int
> -nfs_cancel_remove_inode(struct nfs_page *req, struct inode *inode)
> +static void nfs_cancel_remove_inode(struct nfs_page *req, struct inode *inode)
>  {
> -	int ret;
> -
> -	if (!test_bit(PG_REMOVE, &req->wb_flags))
> -		return 0;
> -	ret = nfs_page_group_lock(req);
> -	if (ret)
> -		return ret;
>  	if (test_and_clear_bit(PG_REMOVE, &req->wb_flags))
>  		nfs_page_set_inode_ref(req, inode);
> -	nfs_page_group_unlock(req);
> -	return 0;
>  }
>  
>  /**
> @@ -585,19 +575,18 @@ static struct nfs_page *nfs_lock_and_join_requests(struct folio *folio)
>  		}
>  	}
>  
> +	ret = nfs_page_group_lock(head);
> +	if (ret < 0)
> +		goto out_unlock;
> +
>  	/* Ensure that nobody removed the request before we locked it */
>  	if (head != folio->private) {
> +		nfs_page_group_unlock(head);
>  		nfs_unlock_and_release_request(head);
>  		goto retry;
>  	}
>  
> -	ret = nfs_cancel_remove_inode(head, inode);
> -	if (ret < 0)
> -		goto out_unlock;
> -
> -	ret = nfs_page_group_lock(head);
> -	if (ret < 0)
> -		goto out_unlock;
> +	nfs_cancel_remove_inode(head, inode);
>  
>  	/* lock each request in the page group */
>  	for (subreq = head->wb_this_page;
> @@ -786,7 +775,8 @@ 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)) {
> +	nfs_page_group_lock(req);
> +	if (nfs_page_group_sync_on_bit_locked(req, PG_REMOVE)) {
>  		struct folio *folio = nfs_page_to_folio(req->wb_head);
>  		struct address_space *mapping = folio->mapping;
>  
> @@ -798,6 +788,7 @@ static void nfs_inode_remove_request(struct nfs_page *req)
>  		}
>  		spin_unlock(&mapping->i_private_lock);
>  	}
> +	nfs_page_group_unlock(req);
>  
>  	if (test_and_clear_bit(PG_INODE_REF, &req->wb_flags)) {
>  		atomic_long_dec(&nfsi->nrequests);
> diff --git a/include/linux/nfs_page.h b/include/linux/nfs_page.h
> index 169b4ae30ff4..9aed39abc94b 100644
> --- a/include/linux/nfs_page.h
> +++ b/include/linux/nfs_page.h
> @@ -160,6 +160,7 @@ extern void nfs_join_page_group(struct nfs_page *head,
>  extern int nfs_page_group_lock(struct nfs_page *);
>  extern void nfs_page_group_unlock(struct nfs_page *);
>  extern bool nfs_page_group_sync_on_bit(struct nfs_page *, unsigned int);
> +extern bool nfs_page_group_sync_on_bit_locked(struct nfs_page *, unsigned int);
>  extern	int nfs_page_set_headlock(struct nfs_page *req);
>  extern void nfs_page_clear_headlock(struct nfs_page *req);
>  extern bool nfs_async_iocounter_wait(struct rpc_task *, struct nfs_lock_context *);

-- 
Jeff Layton <jlayton@kernel.org>