xfs
[Top] [All Lists]

Re: [PATCH 2/4] xfs: kill b_file_offset

To: Christoph Hellwig <hch@xxxxxxxxxxxxx>
Subject: Re: [PATCH 2/4] xfs: kill b_file_offset
From: Dave Chinner <david@xxxxxxxxxxxxx>
Date: Thu, 29 Mar 2012 11:30:29 +1100
Cc: xfs@xxxxxxxxxxx
In-reply-to: <20120328152028.GA12205@xxxxxxxxxxxxx>
References: <1332911958-5613-1-git-send-email-david@xxxxxxxxxxxxx> <1332911958-5613-3-git-send-email-david@xxxxxxxxxxxxx> <20120328152028.GA12205@xxxxxxxxxxxxx>
User-agent: Mutt/1.5.21 (2010-09-15)
On Wed, Mar 28, 2012 at 11:20:28AM -0400, Christoph Hellwig wrote:
> Looks good.
> 
> Reviewed-by: Christoph Hellwig <hch@xxxxxx>
> 
> 
> Minor comments below:
> 
> On Wed, Mar 28, 2012 at 04:19:16PM +1100, Dave Chinner wrote:
> > -   end = bp->b_file_offset + bp->b_buffer_length;
> > -   page_count = xfs_buf_btoc(end) - xfs_buf_btoct(bp->b_file_offset);
> > +   end = BBTOB(bp->b_bn) + bp->b_buffer_length;
> > +   page_count = xfs_buf_btoc(end) - xfs_buf_btoct(BBTOB(bp->b_bn));
> 
> Btw, xfs_buf_btoc and xfs_buf_btoct are more remoal candidates,
> they actually make the code harder to read compared to using the
> opencoded arithmetics..

Ok, I'll consider doing that in the next set of patches.

> >     numbytes = numblks << BBSHIFT;
> >  
> >     /* Check for IOs smaller than the sector size / not sector aligned */
> >     ASSERT(!(numbytes < (1 << btp->bt_sshift)));
> > -   ASSERT(!(offset & (xfs_off_t)btp->bt_smask));
> > +   ASSERT(!(BBTOB(blkno) & (xfs_off_t)btp->bt_smask));
> 
> I don't think it makes sense to keep this assert, given that the
> return value from BBTOB is aligned by defintion.

I kept them there for the 4k sector case. Basic Blocks are always
512 bytes, so for filesystems with sector sizes greater than 512
bytes we still need a sector alignment and size check.

Cheers,

Dave.
-- 
Dave Chinner
david@xxxxxxxxxxxxx

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