Re: [PATCH v4 4/9] dax: fix mapping lifetime handling, convert to __pfn_t + kmap_atomic_pfn_t()

From: Dan Williams
Date: Fri Aug 07 2015 - 19:54:16 EST


On Sat, Jun 6, 2015 at 4:58 AM, Matthew Wilcox <willy@xxxxxxxxxxxxxxx> wrote:
> On Fri, Jun 05, 2015 at 05:19:24PM -0400, Dan Williams wrote:
>> @@ -35,13 +35,16 @@ int dax_clear_blocks(struct inode *inode, sector_t block, long size)
>> might_sleep();
>> do {
>> void *addr;
>> - unsigned long pfn;
>> + __pfn_t pfn;
>> long count;
>>
>> - count = bdev_direct_access(bdev, sector, &addr, &pfn, size);
>> + count = bdev_direct_access(bdev, sector, &pfn, size);
>> if (count < 0)
>> return count;
>> BUG_ON(size < count);
>> + addr = kmap_atomic_pfn_t(pfn);
>> + if (!addr)
>> + return -EIO;
>> while (count > 0) {
>> unsigned pgsz = PAGE_SIZE - offset_in_page(addr);
>> if (pgsz > count)
>
> This part is incomplete. When bdev_direct_access() could return an
> address, it was possible for that address to be unaligned (eg when
> 'sector' was not a multiple of 8). DAX has never had full support for
> devices that weren't a 4k sector size, but I was trying to not make that
> assumption in more places than I had to. So this function needs a lot
> more simplification (or it needs to add '(sector & 7) << 9' to addr ...
> assuming that the partition this bdev represents actually starts at a
> multiple of 8 ... bleh!).

Isn't this already handled by the:

if (sector % (PAGE_SIZE / 512))
return -EINVAL;

...check in bdev_direct_access()? As long as the driver's mapping is
4K aligned, which appears to be the case for all DAX-enabled drivers,
then we should be good to go.

>>
>> -static long dax_get_addr(struct buffer_head *bh, void **addr, unsigned blkbits)
>> +static long dax_get_pfn(struct buffer_head *bh, __pfn_t *pfn, unsigned blkbits)
>> {
>> - unsigned long pfn;
>> sector_t sector = bh->b_blocknr << (blkbits - 9);
>> - return bdev_direct_access(bh->b_bdev, sector, addr, &pfn, bh->b_size);
>> + return bdev_direct_access(bh->b_bdev, sector, pfn, bh->b_size);
>> }
>
> This function should just be deleted. It offers essentially nothing
> over just calling bdev_direct_access().

Ok.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/