xfs
[Top] [All Lists]

Re: [PATCH 02/12] xfs: Remove the macro XFS_BUF_ZEROFLAGS

To: Christoph Hellwig <hch@xxxxxxxxxxxxx>
Subject: Re: [PATCH 02/12] xfs: Remove the macro XFS_BUF_ZEROFLAGS
From: Chandra Seetharaman <sekharan@xxxxxxxxxx>
Date: Thu, 21 Jul 2011 10:29:51 -0700
Cc: xfs@xxxxxxxxxxx
In-reply-to: <20110716015901.GC589@xxxxxxxxxxxxx>
Organization: IBM
References: <20110716012105.6629.24407.sendpatchset@xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx> <20110716012116.6629.48217.sendpatchset@xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx> <20110716015901.GC589@xxxxxxxxxxxxx>
Reply-to: sekharan@xxxxxxxxxx
HI Christoph,

Some clarifications sought....

On Fri, 2011-07-15 at 21:59 -0400, Christoph Hellwig wrote:
> > --- a/fs/xfs/xfs_log.c
> > +++ b/fs/xfs/xfs_log.c
> > @@ -1367,7 +1367,7 @@ xlog_sync(xlog_t              *log,
> >     }
> >     XFS_BUF_SET_COUNT(bp, count);
> >     XFS_BUF_SET_FSPRIVATE(bp, iclog);       /* save for later */
> > -   XFS_BUF_ZEROFLAGS(bp);
> > +   bp->b_flags &= ~(XBF_READ|XBF_WRITE|XBF_ASYNC|XBF_DELWRI|XBF_ORDERED);
> >     XFS_BUF_BUSY(bp);
> >     XFS_BUF_ASYNC(bp);
> >     bp->b_flags |= XBF_LOG_BUFFER;
> 
> Note that XBF_ORDERED is gone in the current tree, you need to rebase
> your patches to pick up the buffer changes I recently sent out.
> 
> I don't link this blind expansion.  For one all that flag setting is
> duplicated in two places in xlog_sync, please send a patch to
> consolidate it in xlog_bdstrat.  Second XBF_READ and XBF_DELWRI

The two places are using 2 different bps, how do you want it to be
consolidated in xlog_bdstrat() ?

> won't ever be set here, so no need to clear them.  And XBF_WRITE
> and XBF_ASYNC will get re-set a few lines down the code, so no need
> to clear them either.  The XBF_FLUSH and XBF_FUA flag also get set
> conditionally a little below, so I'd just clear them in an else
> clause for the if condition checking if they should be set.

It looks cleaner if I just have those flags cleared above, instead of
conditionally clearing them, what do you think ?

> 
> > diff --git a/fs/xfs/xfs_log_recover.c b/fs/xfs/xfs_log_recover.c
> > index 04142ca..75ba081 100644
> > --- a/fs/xfs/xfs_log_recover.c
> > +++ b/fs/xfs/xfs_log_recover.c
> > @@ -261,7 +261,7 @@ xlog_bwrite(
> >     ASSERT(BBTOB(nbblks) <= XFS_BUF_SIZE(bp));
> >  
> >     XFS_BUF_SET_ADDR(bp, log->l_logBBstart + blk_no);
> > -   XFS_BUF_ZEROFLAGS(bp);
> > +   bp->b_flags &= ~(XBF_READ|XBF_WRITE|XBF_ASYNC|XBF_DELWRI|XBF_ORDERED);
> >     XFS_BUF_BUSY(bp);
> >     XFS_BUF_HOLD(bp);
> >     XFS_BUF_PSEMA(bp, PRIBIO);
> 
> XBF_READ, XBF_WRITE, XBF_ASYNC and XBF_DELWRI already get dealt with
> in xfs_bwrite a few lines down, and XBF_ORDERED can't be set here. IOW
> this line can simply be removed.

New definition of XFS_BUF_ZEROFLAGS has these: XBF_SYNCIO, XBF_FUA, and
XBF_FLUSH. In this context is it ok to not clear those ?

> 


<Prev in Thread] Current Thread [Next in Thread>