Re: [PATCH 4/8] xfs: don't log the inode in xfs_fs_map_blocks if it wasn't modified

["Darrick J. Wong" <darrick.wong@oracle.com>]

On Tue, Oct 29, 2019 at 08:58:43AM +0100, Christoph Hellwig wrote:
> On Mon, Oct 28, 2019 at 09:12:45AM -0700, Darrick J. Wong wrote:
> > On Fri, Oct 25, 2019 at 05:03:32PM +0200, Christoph Hellwig wrote:
> > > Even if we are asked for a write layout there is no point in logging
> > > the inode unless we actually modified it in some way.
> > > 
> > > Signed-off-by: Christoph Hellwig <hch@lst.de>
> > > ---
> > >  fs/xfs/xfs_pnfs.c | 43 +++++++++++++++++++------------------------
> > >  1 file changed, 19 insertions(+), 24 deletions(-)
> > > 
> > > diff --git a/fs/xfs/xfs_pnfs.c b/fs/xfs/xfs_pnfs.c
> > > index 9c96493be9e0..fa90c6334c7c 100644
> > > --- a/fs/xfs/xfs_pnfs.c
> > > +++ b/fs/xfs/xfs_pnfs.c
> > > @@ -147,32 +147,27 @@ xfs_fs_map_blocks(
> > >  	if (error)
> > >  		goto out_unlock;
> > >  
> > > -	if (write) {
> > > -		enum xfs_prealloc_flags	flags = 0;
> > > -
> > > +	if (write &&
> > > +	    (!nimaps || imap.br_startblock == HOLESTARTBLOCK)) {
> > >  		ASSERT(imap.br_startblock != DELAYSTARTBLOCK);
> > 
> > The change in code flow makes this assert rather useless, I think, since
> > we only end up in this branch if we have a write and a hole.  If the
> > condition that it checks is important (and it seems to be?) then it
> > ought to be hoisted up a level and turned into:
> > 
> > ASSERT(!write || !nimaps || imap.br_startblock != DELAYSTARTBLOCK);
> > 
> > Right?
> 
> Actually even for !write we should not see delalloc blocks here.
> So I'll fix up the assert in a separate prep patch.

<shrug> I could just fix it, unless you're about to resend the whole series?

--D