Hey,
On Mon, Dec 10, 2012 at 12:59:11PM +1100, Dave Chinner wrote:
> On Sat, Dec 08, 2012 at 07:38:21AM -0500, Christoph Hellwig wrote:
> > On Tue, Dec 04, 2012 at 05:18:05PM -0600, Mark Tinguely wrote:
> > > Per Dave Chinner suggestion, this patch:
> > > 1) Corrects the detection of whether a multi-segment buffer is
> > > still tracking data.
> > > 2) Clears all the buffer log formats for a multi-segment buffer.
> > >
> > > Signed-off-by: Mark Tinguely <tinguely@xxxxxxx>
> > > Reviewed-by: Christoph Hellwig <hch@xxxxxx>
> > > ---
> > > fs/xfs/xfs_buf_item.c | 13 ++++++++++---
> > > fs/xfs/xfs_buf_item.c | 13 ++++++++++---
> > > fs/xfs/xfs_trans_buf.c | 7 +++++--
> > > 2 files changed, 15 insertions(+), 5 deletions(-)
> > >
> > > Index: b/fs/xfs/xfs_buf_item.c
> > > ===================================================================
> > > --- a/fs/xfs/xfs_buf_item.c
> > > +++ b/fs/xfs/xfs_buf_item.c
> > > @@ -611,7 +611,7 @@ xfs_buf_item_unlock(
> > > {
> > > struct xfs_buf_log_item *bip = BUF_ITEM(lip);
> > > struct xfs_buf *bp = bip->bli_buf;
> > > - int aborted;
> > > + int aborted, clean, i;
> > > uint hold;
> > >
> > > /* Clear the buffer's association with this transaction. */
> > > @@ -654,8 +654,15 @@ xfs_buf_item_unlock(
> > > * If the buf item isn't tracking any data, free it, otherwise drop the
> > > * reference we hold to it.
> > > */
> > > - if (xfs_bitmap_empty(bip->__bli_format.blf_data_map,
> > > - bip->__bli_format.blf_map_size))
> > > + clean = 1;
> > > + for (i = 0; i < bip->bli_format_count; i++) {
> > > + if (!xfs_bitmap_empty(bip->bli_formats[i].blf_data_map,
> > > + bip->bli_formats[i].blf_map_size)) {
> > > + clean = 0;
> > > + break;
> > > + }
> > > + }
> > > + if (clean)
> > > xfs_buf_item_relse(bp);
> > > else
> > > atomic_dec(&bip->bli_refcount);
> >
> > Looks ok, although avoiding the clean variable would be even better:
> >
> > for (i = 0; i < bip->bli_format_count; i++) {
> > if (!xfs_bitmap_empty(bip->bli_formats[i].blf_data_map,
> > bip->bli_formats[i].blf_map_size)) {
> > atomic_dec(&bip->bli_refcount);
> > goto out;
> > }
> > }
> >
> > xfs_buf_item_relse(bp);
> > out:
>
> Definitely better.
>
> >
> > bu that might be getting a bit too much into bikeshedding.
> >
> > What I'm worried more about is how we semi-duplicate this bli_refcount
> > decrement vs xfs_buf_item_relse in xfs_trans_brelse, but use the
> > xfs_buf_item_dirty (aka XFS_BLI_DIRTY) check there instead.
>
> Well, we only get to the case in xfs_trans_brelse() if the buffer
> was not modified in this transaction. Hence the check is for whether
> it was modified in a previous transaction (and hence is in the AIL)
> and not whether the buffer has any changes in the bitmap or not.
>
> So to me the checks seem to be for two different cases - one if so
> whether the buffer has physical changes, the other for whether it is
> currently in the AIL.
>
> A further complication is that the XFS_BLI_DIRTY flag is cleared
> when the buffer is marked stale, so any path that looks at this flag
> needs to specifically handle the XFS_BLI_STALE case before the dirty
> case.
>
> It seems to me that the one place that XFS_BLI_DIRTY is checked
> could actually be replaced with a:
>
> if (!(bip->bli_item.li_flags & XFS_LI_IN_AIL)) {
> ....
> }
>
> check and hence remove the reason for it's existence completely.
> At that point, the flag could be repurposed as you suggest here:
>
> > It seems like the proper fix might be to:
> >
> > - only set XFS_BLI_DIRTY in xfs_buf_item_log if we actually set
> > any bits in a bitmap
> > - use the XFS_BLI_DIRTY check in xfs_buf_item_unlock as well
> > - kill the useless xfs_buf_item_dirty wrapper
> >
> > Probably both of these aren't worth doing it for now as we'll need to
> > get fixes into Linus tree quickly, so:
>
> Agreed.
I feel they are appropriate for 3.8-rc1 during the merge window. Any
objections? If you gentlemen wish, I can wait for -rc2.
Regards,
Ben
_______________________________________________
xfs mailing list
xfs@xxxxxxxxxxx
http://oss.sgi.com/mailman/listinfo/xfs
