Received-SPF: pass (google.com: domain of linux-nfs+bounces-2227-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
Content-Type: text/plain; charset="utf-8"
Content-Transfer-Encoding: 7bit
Precedence: bulk
MIME-Version: 1.0
From: "NeilBrown" <neilb@suse.de>
To: "Jeff Layton" <jlayton@kernel.org>
Cc: stable@vger.kernel.org,
 "Trond Myklebust" <trond.myklebust@hammerspace.com>,
 "Anna Schumaker" <anna@kernel.org>, linux-nfs@vger.kernel.org,
 "Dan Aloni" <dan.aloni@vastdata.com>
Subject:
 Re: [PATCH stable 6.6 and 6.7] NFS: Fix data corruption caused by congestion.
In-reply-to: <cfec488eccfc3469d18dd94b05a00919cc152113.camel@kernel.org>
References: <170907621128.24797.4390391329078744015@noble.neil.brown.name>,
 <cfec488eccfc3469d18dd94b05a00919cc152113.camel@kernel.org>
Date: Thu, 07 Mar 2024 22:41:41 +1100
Message-id: <170981170160.13576.347273159851012933@noble.neil.brown.name>

On Thu, 07 Mar 2024, Jeff Layton wrote:
> On Wed, 2024-02-28 at 10:23 +1100, NeilBrown wrote:
> > when AOP_WRITEPAGE_ACTIVATE is returned (as NFS does when it detects
> > congestion) it is important that the folio is redirtied.
> > nfs_writepage_locked() doesn't do this, so files can become corrupted as
> > writes can be lost.
> > 
> > Note that this is not needed in v6.8 as AOP_WRITEPAGE_ACTIVATE cannot be
> > returned.  It is needed for kernels v5.18..v6.7.  Prior to 6.3 the patch
> > is different as it needs to mention "page", not "folio".
> > 
> 
> Neil, I have a question about the above statement. In Linus's tree as of
> this morning (v6.8-rc7-ish), it does this in nfs_writepages_locked:
> 
>         if (wbc->sync_mode == WB_SYNC_NONE &&
>             NFS_SERVER(inode)->write_congested)           
>                 return AOP_WRITEPAGE_ACTIVATE;
> 
> The only caller of nfs_writepages_locked, and I don't see where it
> redirties the page. Why don't we need this in v6.8?

You are right - it doesn't redirty anything.  But there is no bug
here....
I didn't see it at first either, but the only caller of
nfs_writepage_locked() is nfs_wb_folio() (as you say) and that always
passes a wbc with .sync_mode = WB_SYNC_ALL.  So sync_mode is never
WB_SYNC_NODE and the code snippet you included above is dead code.  I've
already posted a patch to Trond and Anna to remove that code.

Thanks for the review!

NeilBrown

> 
> 
> > Reported-and-tested-by: Jacek Tomaka <Jacek.Tomaka@poczta.fm>
> > Fixes: 6df25e58532b ("nfs: remove reliance on bdi congestion")
> > Signed-off-by: NeilBrown <neilb@suse.de>
> > ---
> >  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 b664caea8b4e..9e345d3c305a 100644
> > --- a/fs/nfs/write.c
> > +++ b/fs/nfs/write.c
> > @@ -668,8 +668,10 @@ static int nfs_writepage_locked(struct folio *folio,
> >  	int err;
> >  
> >  	if (wbc->sync_mode == WB_SYNC_NONE &&
> > -	    NFS_SERVER(inode)->write_congested)
> > +	    NFS_SERVER(inode)->write_congested) {
> > +		folio_redirty_for_writepage(wbc, folio);
> >  		return AOP_WRITEPAGE_ACTIVATE;
> > +	}
> >  
> >  	nfs_inc_stats(inode, NFSIOS_VFSWRITEPAGE);
> >  	nfs_pageio_init_write(&pgio, inode, 0, false,
> 
> -- 
> Jeff Layton <jlayton@kernel.org>
>