Re: [resend PATCH v2 08/33] dcssblk: add dax_operations support

[Dan Williams]

On Wed, Apr 19, 2017 at 8:31 AM, Gerald Schaefer
<gerald.schaefer@xxxxxxxxxx> wrote:
> On Mon, 17 Apr 2017 12:09:32 -0700
> Dan Williams <dan.j.williams@xxxxxxxxx> wrote:
>
>> Setup a dax_dev to have the same lifetime as the dcssblk block device
>> and add a ->direct_access() method that is equivalent to
>> dcssblk_direct_access(). Once fs/dax.c has been converted to use
>> dax_operations the old dcssblk_direct_access() will be removed.
>>
>> Cc: Gerald Schaefer <gerald.schaefer@xxxxxxxxxx>
>> Signed-off-by: Dan Williams <dan.j.williams@xxxxxxxxx>
>> ---
>>  drivers/s390/block/Kconfig   |    1 +
>>  drivers/s390/block/dcssblk.c |   54 +++++++++++++++++++++++++++++++++++-------
>>  2 files changed, 46 insertions(+), 9 deletions(-)
>>
>> diff --git a/drivers/s390/block/Kconfig b/drivers/s390/block/Kconfig
>> index 4a3b62326183..0acb8c2f9475 100644
>> --- a/drivers/s390/block/Kconfig
>> +++ b/drivers/s390/block/Kconfig
>> @@ -14,6 +14,7 @@ config BLK_DEV_XPRAM
>>
>>  config DCSSBLK
>>       def_tristate m
>> +     select DAX
>>       prompt "DCSSBLK support"
>>       depends on S390 && BLOCK
>>       help
>> diff --git a/drivers/s390/block/dcssblk.c b/drivers/s390/block/dcssblk.c
>> index 415d10a67b7a..682a9eb4934d 100644
>> --- a/drivers/s390/block/dcssblk.c
>> +++ b/drivers/s390/block/dcssblk.c
>> @@ -18,6 +18,7 @@
>>  #include <linux/interrupt.h>
>>  #include <linux/platform_device.h>
>>  #include <linux/pfn_t.h>
>> +#include <linux/dax.h>
>>  #include <asm/extmem.h>
>>  #include <asm/io.h>
>>
>> @@ -30,8 +31,10 @@ static int dcssblk_open(struct block_device *bdev, fmode_t mode);
>>  static void dcssblk_release(struct gendisk *disk, fmode_t mode);
>>  static blk_qc_t dcssblk_make_request(struct request_queue *q,
>>                                               struct bio *bio);
>> -static long dcssblk_direct_access(struct block_device *bdev, sector_t secnum,
>> +static long dcssblk_blk_direct_access(struct block_device *bdev, sector_t secnum,
>>                        void **kaddr, pfn_t *pfn, long size);
>> +static long dcssblk_dax_direct_access(struct dax_device *dax_dev, pgoff_t pgoff,
>> +             long nr_pages, void **kaddr, pfn_t *pfn);
>>
>>  static char dcssblk_segments[DCSSBLK_PARM_LEN] = "\0";
>>
>> @@ -40,7 +43,11 @@ static const struct block_device_operations dcssblk_devops = {
>>       .owner          = THIS_MODULE,
>>       .open           = dcssblk_open,
>>       .release        = dcssblk_release,
>> -     .direct_access  = dcssblk_direct_access,
>> +     .direct_access  = dcssblk_blk_direct_access,
>> +};
>> +
>> +static const struct dax_operations dcssblk_dax_ops = {
>> +     .direct_access = dcssblk_dax_direct_access,
>>  };
>>
>>  struct dcssblk_dev_info {
>> @@ -57,6 +64,7 @@ struct dcssblk_dev_info {
>>       struct request_queue *dcssblk_queue;
>>       int num_of_segments;
>>       struct list_head seg_list;
>> +     struct dax_device *dax_dev;
>>  };
>>
>>  struct segment_info {
>> @@ -389,6 +397,8 @@ dcssblk_shared_store(struct device *dev, struct device_attribute *attr, const ch
>>       }
>>       list_del(&dev_info->lh);
>>
>> +     kill_dax(dev_info->dax_dev);
>> +     put_dax(dev_info->dax_dev);
>>       del_gendisk(dev_info->gd);
>>       blk_cleanup_queue(dev_info->dcssblk_queue);
>>       dev_info->gd->queue = NULL;
>> @@ -525,6 +535,7 @@ dcssblk_add_store(struct device *dev, struct device_attribute *attr, const char
>>       int rc, i, j, num_of_segments;
>>       struct dcssblk_dev_info *dev_info;
>>       struct segment_info *seg_info, *temp;
>> +     struct dax_device *dax_dev;
>>       char *local_buf;
>>       unsigned long seg_byte_size;
>>
>> @@ -654,6 +665,11 @@ dcssblk_add_store(struct device *dev, struct device_attribute *attr, const char
>>       if (rc)
>>               goto put_dev;
>>
>> +     dax_dev = alloc_dax(dev_info, dev_info->gd->disk_name,
>> +                     &dcssblk_dax_ops);
>> +     if (!dax_dev)
>> +             goto put_dev;
>> +
>
> The returned dax_dev should be stored into dev_info->dax_dev, for later use
> by kill/put_dax(). This can also be done directly, so that we don't need the
> local dax_dev variable here.
>
> Also, in the error case, a proper rc should be set before going to put_dev,
> probably -ENOMEM.
>
> I took a quick look at the patches for the other affected drivers, and it
> looks like axonram also has the "missing rc" issue, and brd the "missing
> brd->dax_dev init" issue, pmem seems to be fine.

Thank you for taking a look. I'll get this fixed up.