On 11/23/2012 02:19 AM, Christoph Hellwig wrote: > On Wed, Nov 21, 2012 at 04:40:49PM -0600, Dave Kleikamp wrote: >> From: Zach Brown <[email protected]> >> >> __blockdev_direct_IO() had two instances of the same code to determine >> if a given offset wasn't aligned first to the inode's blkbits and then >> to the underlying device's blkbits. This was confusing enough but >> we're about to add code that performs the same check on offsets in bvec >> arrays. Rather than add yet more copies of this code let's have >> everyone call a helper. >> >> Signed-off-by: Dave Kleikamp <[email protected]> >> Cc: Zach Brown <[email protected]> >> --- >> fs/direct-io.c | 59 >> ++++++++++++++++++++++++++++++++++++---------------------- >> 1 file changed, 37 insertions(+), 22 deletions(-) >> >> diff --git a/fs/direct-io.c b/fs/direct-io.c >> index f86c720..035c0a3 100644 >> --- a/fs/direct-io.c >> +++ b/fs/direct-io.c >> @@ -1020,6 +1020,39 @@ static inline int drop_refcount(struct dio *dio) >> } >> >> /* >> + * Returns true if the given offset is aligned to either the IO size >> + * specified by the given blkbits or by the logical block size of the >> + * given block device. >> + * >> + * If the given offset isn't aligned to the blkbits arguments as this is >> + * called then blkbits is set to the block size of the specified block >> + * device. The call can then return either true or false. >> + * >> + * This bizarre calling convention matches the code paths that >> + * duplicated the functionality that this helper was built from. We >> + * reproduce the behaviour to avoid introducing subtle bugs. >> + */ >> +static int dio_aligned(unsigned long offset, unsigned *blkbits, >> + struct block_device *bdev) >> +{ >> + unsigned mask = (1 << *blkbits) - 1; >> + >> + /* >> + * Avoid references to bdev if not absolutely needed to give >> + * the early prefetch in the caller enough time. >> + */ >> + >> + if (offset & mask) { >> + if (bdev) >> + *blkbits = blksize_bits(bdev_logical_block_size(bdev)); > > I don't like having the blkbits assignment hidden in the helper, > shouldn't we have a: > > if (bdev) > blkbits = blksize_bits(bdev_logical_block_size(bdev)); > else > blkbits = inode->i_blkbits; > > in the caller instead?
Yeah, that absolutely makes more sense. Shaggy -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
