xfs
[Top] [All Lists]

Re: [PATCH 3/4] xfs: use blocks for counting length of buffers

To: Christoph Hellwig <hch@xxxxxxxxxxxxx>
Subject: Re: [PATCH 3/4] xfs: use blocks for counting length of buffers
From: Dave Chinner <david@xxxxxxxxxxxxx>
Date: Thu, 29 Mar 2012 11:31:36 +1100
Cc: xfs@xxxxxxxxxxx
In-reply-to: <20120328152639.GB12205@xxxxxxxxxxxxx>
References: <1332911958-5613-1-git-send-email-david@xxxxxxxxxxxxx> <1332911958-5613-4-git-send-email-david@xxxxxxxxxxxxx> <20120328152639.GB12205@xxxxxxxxxxxxx>
User-agent: Mutt/1.5.21 (2010-09-15)
On Wed, Mar 28, 2012 at 11:26:39AM -0400, Christoph Hellwig wrote:
> > +                   tmp = (valuelen < BBTOB(bp->b_length))
> > +                           ? valuelen : BBTOB(bp->b_length);
> 
> maybe use min or min_t here while you're at it?
> 
> > -           tmp = (valuelen < XFS_BUF_SIZE(bp)) ? valuelen :
> > -                                                   XFS_BUF_SIZE(bp);
> > +           tmp = valuelen < BBTOB(bp->b_length) ? valuelen :
> > +                                                  BBTOB(bp->b_length);
> 
> Same here.
> 
> >             xfs_buf_iomove(bp, 0, tmp, src, XBRW_WRITE);
> > -           if (tmp < XFS_BUF_SIZE(bp))
> > -                   xfs_buf_zero(bp, tmp, XFS_BUF_SIZE(bp) - tmp);
> > +           if (tmp < BBTOB(bp->b_length))
> > +                   xfs_buf_zero(bp, tmp, BBTOB(bp->b_length) - tmp);
> 
> Also a local buf_len variable in this function would probably be useful.
> 
> > -   size_t                  b_buffer_length;/* size of buffer in bytes */
> > +   size_t                  b_length;       /* size of buffer in BBs */
> 
> A count of blocks probably shold not be a size_t, but a uint.
> 
> >     TP_fast_assign(
> >             __entry->dev = bp->b_target->bt_dev;
> >             __entry->bno = bp->b_bn;
> > -           __entry->buffer_length = bp->b_buffer_length;
> > +           __entry->buffer_length = BBTOB(bp->b_length);
> 
> Given that we print the bno in blocks it might make sense to print
> this as number of blocks, too?  Change the description string to nblks
> in that case as well.

All makes sense. Will do.

Cheers,

Dave.
-- 
Dave Chinner
david@xxxxxxxxxxxxx

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