On Mon, Nov 9, 2015 at 12:06 PM, Matthew Wilcox <willy@xxxxxxxxxxxxxxx> wrote:
> On Sun, Nov 08, 2015 at 02:27:44PM -0500, Dan Williams wrote:
>> +++ b/fs/block_dev.c
>> @@ -464,7 +464,7 @@ long bdev_direct_access(struct block_device *bdev, sector_t sector,
>> if (sector % (PAGE_SIZE / 512))
>> return -EINVAL;
>> avail = ops->direct_access(bdev, sector, addr, pfn);
>> - if (!avail)
>> + if (!avail || avail < PAGE_SIZE)
>> return -ERANGE;
>> return min(avail, size);
>> }
>
> ... redundant? avail == 0 is covered by avail < PAGE_SIZE.
>
>> diff --git a/fs/dax.c b/fs/dax.c
>> index 149d6000d72a..b1ac67de0654 100644
>> --- a/fs/dax.c
>> +++ b/fs/dax.c
>> @@ -30,6 +30,40 @@
>> #include <linux/vmstat.h>
>> #include <linux/sizes.h>
>>
>> +static void __pmem *__dax_map_atomic(struct block_device *bdev, sector_t sector,
>> + long size, unsigned long *pfn, long *len)
>> +{
>> + long rc;
>> + void __pmem *addr;
>> + struct request_queue *q = bdev->bd_queue;
>> +
>> + if (blk_queue_enter(q, GFP_NOWAIT) != 0)
>> + return (void __pmem *) ERR_PTR(-EIO);
>> + rc = bdev_direct_access(bdev, sector, &addr, pfn, size);
>> + if (len)
>> + *len = rc;
>> + if (rc < 0) {
>> + blk_queue_exit(q);
>> + return (void __pmem *) ERR_PTR(rc);
>> + }
>> + return addr;
>> +}
>
> I don't like this helper function. You've changed the calling convention to
> 'return a pointer, which might be an error value' from the rest of the
> DAX code which is using 'return a length; if it's negative that's an error'.
>
>> @@ -42,9 +76,9 @@ int dax_clear_blocks(struct inode *inode, sector_t block, long size)
>> long count, sz;
>>
>> sz = min_t(long, size, SZ_1M);
>> - count = bdev_direct_access(bdev, sector, &addr, &pfn, sz);
>> - if (count < 0)
>> - return count;
>> + addr = __dax_map_atomic(bdev, sector, size, &pfn, &count);
>> + if (IS_ERR(addr))
>> + return PTR_ERR(addr);
>
> I don't have anything against the PTR_ERR/ERR_PTR convention per se,
> but you've complicated the callers, plus there's now this awkward bit
> in the middle of the code where the convention suddenly changes. Since
> you need to return both the length and the address from the function,
> one of them has to be passed by reference.
>
Yeah, I can't disagree with the assessment, but the pass by reference
output value troubles are also in ->direct_access() and I think the
whole thing wants cleaning up. I recently came up with blk_dax_ctl
[1], and while Dave hates passing a 'flags' value I think the rest of
it is a nice clean up. The question is are you NAK'ing this one or
asking for further cleanups later. I have this sitting on
libnvdimm-for-next, and if I need to revise this series again I think
I'll just need to drop the whole topic branch for 4.4 and try again in
4.5.
[1]: https://lists.01.org/pipermail/linux-nvdimm/2015-November/002674.html
--
To unsubscribe from this list: send the line "unsubscribe linux-block" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
