[PATCH 0/2] Support DAX for device-mapper dm-linear devices

[PATCH 0/2] Support DAX for device-mapper dm-linear devices

[Toshi Kani]

I noticed that dm-snap reloads DM table of target mapped-device, which
fails for dax-capable device after dax support is added.  Ideally,
adding dax support to dm-snap solves the issue, but it cannot be done
easily.  This patch-set allows dm-snap to work with dax-capable target
devices when bio-based operation is used.

dax operation is unsupported with dm-snap, such that:
 a) After snapshot is taken, mount with dax option to a target device
    or a snapshot device fails.  They can be mounted without dax.
 b) After snapshot is taken to a dax-mounted target device, any writes 
    to the target device fails (EIO).

b) can be protected by changing lvcreate to fail when snapshot is
requested to a dax-mounted target device. 

- Patch 1 solves an error when lvremove is made to a snapshot device.
- Patch 2 solves an error when lvcreate --snapshot is made to a dax-
  capable device.

---
Toshi Kani (2):
 1/2 dm: update table type check for dax
 2/2 dm snap: add fake origin_direct_access

---
 drivers/md/dm-ioctl.c | 11 ++++++++++-
 drivers/md/dm-snap.c  |  8 ++++++++
 2 files changed, 18 insertions(+), 1 deletion(-)

[PATCH 1/2] dm: update table type check for dax

[Toshi Kani]

Allow table type DM_TYPE_BIO_BASED to extend with
DM_TYPE_DAX_BIO_BASED since DM_TYPE_DAX_BIO_BASED
supports bio-based requests.

Signed-off-by: Toshi Kani <toshi.kani@hpe.com>
Cc: Mike Snitzer <snitzer@redhat.com>
Cc: Alasdair Kergon <agk@redhat.com>
Cc: Dan Williams <dan.j.williams@intel.com>
---
 drivers/md/dm-ioctl.c |   11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/drivers/md/dm-ioctl.c b/drivers/md/dm-ioctl.c
index b59e3459..0f32791 100644
--- a/drivers/md/dm-ioctl.c
+++ b/drivers/md/dm-ioctl.c
@@ -1267,6 +1267,15 @@ static int populate_table(struct dm_table *table,
 	return dm_table_complete(table);
 }
 
+static bool is_valid_type(unsigned cur, unsigned new)
+{
+	if (cur == new ||
+	    (cur == DM_TYPE_BIO_BASED && new == DM_TYPE_DAX_BIO_BASED))
+		return true;
+
+	return false;
+}
+
 static int table_load(struct dm_ioctl *param, size_t param_size)
 {
 	int r;
@@ -1309,7 +1318,7 @@ static int table_load(struct dm_ioctl *param, size_t param_size)
 			DMWARN("unable to set up device queue for new table.");
 			goto err_unlock_md_type;
 		}
-	} else if (dm_get_md_type(md) != dm_table_get_type(t)) {
+	} else if (!is_valid_type(dm_get_md_type(md), dm_table_get_type(t))) {
 		DMWARN("can't change device type after initial table load.");
 		r = -EINVAL;
 		goto err_unlock_md_type;

Re: [PATCH 1/2] dm: update table type check for dax

[Mike Snitzer]

On Tue, Jun 28 2016 at  3:37pm -0400,
Toshi Kani <toshi.kani@hpe.com> wrote:

> Allow table type DM_TYPE_BIO_BASED to extend with
> DM_TYPE_DAX_BIO_BASED since DM_TYPE_DAX_BIO_BASED
> supports bio-based requests.
> 
> Signed-off-by: Toshi Kani <toshi.kani@hpe.com>
> Cc: Mike Snitzer <snitzer@redhat.com>
> Cc: Alasdair Kergon <agk@redhat.com>
> Cc: Dan Williams <dan.j.williams@intel.com>
> ---
>  drivers/md/dm-ioctl.c |   11 ++++++++++-
>  1 file changed, 10 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/md/dm-ioctl.c b/drivers/md/dm-ioctl.c
> index b59e3459..0f32791 100644
> --- a/drivers/md/dm-ioctl.c
> +++ b/drivers/md/dm-ioctl.c
> @@ -1267,6 +1267,15 @@ static int populate_table(struct dm_table *table,
>  	return dm_table_complete(table);
>  }
>  
> +static bool is_valid_type(unsigned cur, unsigned new)
> +{
> +	if (cur == new ||
> +	    (cur == DM_TYPE_BIO_BASED && new == DM_TYPE_DAX_BIO_BASED))
> +		return true;
> +
> +	return false;
> +}
> +
>  static int table_load(struct dm_ioctl *param, size_t param_size)
>  {
>  	int r;
> @@ -1309,7 +1318,7 @@ static int table_load(struct dm_ioctl *param, size_t param_size)
>  			DMWARN("unable to set up device queue for new table.");
>  			goto err_unlock_md_type;
>  		}
> -	} else if (dm_get_md_type(md) != dm_table_get_type(t)) {
> +	} else if (!is_valid_type(dm_get_md_type(md), dm_table_get_type(t))) {
>  		DMWARN("can't change device type after initial table load.");
>  		r = -EINVAL;
>  		goto err_unlock_md_type;
> 

You said in the 0th header: "Patch 1 solves an error when lvremove is
made to a snapshot device."

I'm not seeing why this patch 1 fixes anything specific to snapshot
device removal (but I can see why patch 2 makes snapshot creation
"work").  I'll apply your 2nd patch and see if I can see what you mean.

I actually see this error, without either of your 2 proposed patches
applied, when I try to create a snapshot of a DAX capable LV:

# lvcreate -s -n snap -L 100M pmem/lv
  device-mapper: reload ioctl on (253:7) failed: Invalid argument
  Failed to lock logical volume pmem/lv.
  Aborting. Manual intervention required.
Jun 28 15:57:28 rhel-storage-02 kernel: device-mapper: ioctl: can't change device type after initial table load.

Re: [PATCH 1/2] dm: update table type check for dax

[Kani, Toshimitsu]

On Tue, 2016-06-28 at 16:07 -0400, Mike Snitzer wrote:
> On Tue, Jun 28 2016 at  3:37pm -0400,
> Toshi Kani <toshi.kani@hpe.com> wrote:
 :
> You said in the 0th header: "Patch 1 solves an error when lvremove is
> made to a snapshot device."
> 
> I'm not seeing why this patch 1 fixes anything specific to snapshot
> device removal (but I can see why patch 2 makes snapshot creation
> "work").  I'll apply your 2nd patch and see if I can see what you mean.
> 
> I actually see this error, without either of your 2 proposed patches
> applied, when I try to create a snapshot of a DAX capable LV:
> 
> # lvcreate -s -n snap -L 100M pmem/lv
>   device-mapper: reload ioctl on (253:7) failed: Invalid argument
>   Failed to lock logical volume pmem/lv.
>   Aborting. Manual intervention required.
> Jun 28 15:57:28 rhel-storage-02 kernel: device-mapper: ioctl: can't change
> device type after initial table load.

Yes, patch 2 fixes this error.

I have not looked into why lvremove does this, but lvremove to a snapshot
device fails to reload DM table of "<dev>-lvsnap" device (which is marked as
DM_TYPE_BIO_BASED) with DM_TYPE_DAX_BIO_BASED.  Patch 1 fixes this error.  I
think it also generally makes sense to allow this case.

Thanks,
-Toshi

Re: [PATCH 1/2] dm: update table type check for dax

[Mike Snitzer]

On Tue, Jun 28 2016 at  4:23pm -0400,
Kani, Toshimitsu <toshi.kani@hpe.com> wrote:

> On Tue, 2016-06-28 at 16:07 -0400, Mike Snitzer wrote:
> > On Tue, Jun 28 2016 at  3:37pm -0400,
> > Toshi Kani <toshi.kani@hpe.com> wrote:
>  :
> > You said in the 0th header: "Patch 1 solves an error when lvremove is
> > made to a snapshot device."
> > 
> > I'm not seeing why this patch 1 fixes anything specific to snapshot
> > device removal (but I can see why patch 2 makes snapshot creation
> > "work").  I'll apply your 2nd patch and see if I can see what you mean.
> > 
> > I actually see this error, without either of your 2 proposed patches
> > applied, when I try to create a snapshot of a DAX capable LV:
> > 
> > # lvcreate -s -n snap -L 100M pmem/lv
> >   device-mapper: reload ioctl on (253:7) failed: Invalid argument
> >   Failed to lock logical volume pmem/lv.
> >   Aborting. Manual intervention required.
> > Jun 28 15:57:28 rhel-storage-02 kernel: device-mapper: ioctl: can't change
> > device type after initial table load.
> 
> Yes, patch 2 fixes this error.
> 
> I have not looked into why lvremove does this, but lvremove to a snapshot
> device fails to reload DM table of "<dev>-lvsnap" device (which is marked as
> DM_TYPE_BIO_BASED) with DM_TYPE_DAX_BIO_BASED.  Patch 1 fixes this error.

It looks like a strange intermediate state that lvm2 uses during
snapshot removal.

Full listing of snapshot related DM tables (before lvremove):

pmem-lv-real: 0 6086656 linear 259:0 2048
pmem-lv: 0 6086656 snapshot-origin 253:5
pmem-snap-cow: 0 204800 linear 259:0 6088704
pmem-snap: 0 6086656 snapshot 253:5 253:6 P 8

When removing this snapshot we're wanting to be left with:

pmem-lv: 0 6086656 linear 259:0 2048

I augmented the DM core error to be more expressive, resulting in:
  device-mapper: ioctl: 253:7: can't change device type (from 1 to 4) after initial table load.

1 is DM_TYPE_BIO_BASED and 4 is DM_TYPE_DAX_BIO_BASED -- which makes
sense given the linear target is DM_TYPE_DAX_BIO_BASED.

The previous DM table for 253:7 was:
  pmem-snap: 0 6086656 snapshot 253:5 253:6 P 8

The intermediate table that lvm2 is trying to load for 253:7 is:
  0 204800 linear 259:0 6088704

(this linear target was previously pmem-snap-cow)

> I think it also generally makes sense to allow this case.

You're probably right but I need to think about it a little bit more.

Re: [PATCH 1/2] dm: update table type check for dax

[Kani, Toshimitsu]

On Tue, 2016-06-28 at 20:40 -0400, Mike Snitzer wrote:
> On Tue, Jun 28 2016 at  4:23pm -0400,
> Kani, Toshimitsu <toshi.kani@hpe.com> wrote:
> 
> > On Tue, 2016-06-28 at 16:07 -0400, Mike Snitzer wrote:
> > > 
> > > On Tue, Jun 28 2016 at  3:37pm -0400,
> > > Toshi Kani <toshi.kani@hpe.com> wrote:
> >  :
> > > 
> > > You said in the 0th header: "Patch 1 solves an error when lvremove is
> > > made to a snapshot device."
> > > 
> > > I'm not seeing why this patch 1 fixes anything specific to snapshot
> > > device removal (but I can see why patch 2 makes snapshot creation
> > > "work").  I'll apply your 2nd patch and see if I can see what you mean.
> > > 
> > > I actually see this error, without either of your 2 proposed patches
> > > applied, when I try to create a snapshot of a DAX capable LV:
> > > 
> > > # lvcreate -s -n snap -L 100M pmem/lv
> > >   device-mapper: reload ioctl on (253:7) failed: Invalid argument
> > >   Failed to lock logical volume pmem/lv.
> > >   Aborting. Manual intervention required.
> > > Jun 28 15:57:28 rhel-storage-02 kernel: device-mapper: ioctl: can't
> > > change device type after initial table load.
> >
> > Yes, patch 2 fixes this error.
> > 
> > I have not looked into why lvremove does this, but lvremove to a snapshot
> > device fails to reload DM table of "<dev>-lvsnap" device (which is marked
> > as DM_TYPE_BIO_BASED) with DM_TYPE_DAX_BIO_BASED.  Patch 1 fixes this
> > error.
>
> It looks like a strange intermediate state that lvm2 uses during
> snapshot removal.
> 
> Full listing of snapshot related DM tables (before lvremove):
> 
> pmem-lv-real: 0 6086656 linear 259:0 2048
> pmem-lv: 0 6086656 snapshot-origin 253:5
> pmem-snap-cow: 0 204800 linear 259:0 6088704
> pmem-snap: 0 6086656 snapshot 253:5 253:6 P 8
> 
> When removing this snapshot we're wanting to be left with:
> 
> pmem-lv: 0 6086656 linear 259:0 2048
> 
> I augmented the DM core error to be more expressive, resulting in:
>   device-mapper: ioctl: 253:7: can't change device type (from 1 to 4) after
> initial table load.
> 
> 1 is DM_TYPE_BIO_BASED and 4 is DM_TYPE_DAX_BIO_BASED -- which makes
> sense given the linear target is DM_TYPE_DAX_BIO_BASED.
> 
> The previous DM table for 253:7 was:
>   pmem-snap: 0 6086656 snapshot 253:5 253:6 P 8
> 
> The intermediate table that lvm2 is trying to load for 253:7 is:
>   0 204800 linear 259:0 6088704
> 
> (this linear target was previously pmem-snap-cow)

Yes, that is consistent with what I saw.

> > 
> > I think it also generally makes sense to allow this case.
>
> You're probably right but I need to think about it a little bit more.

Sounds good.

Thanks!
-Toshi

Re: [PATCH 1/2] dm: update table type check for dax

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 1565 bytes --]

Hi,

[auto build test ERROR on dm/for-next]
[also build test ERROR on v4.7-rc5 next-20160628]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Toshi-Kani/Support-DAX-for-device-mapper-dm-linear-devices/20160629-034110
base:   https://git.kernel.org/pub/scm/linux/kernel/git/device-mapper/linux-dm.git for-next
config: x86_64-randconfig-s1-06290340 (attached as .config)
compiler: gcc-6 (Debian 6.1.1-1) 6.1.1 20160430
reproduce:
        # save the attached .config to linux build tree
        make ARCH=x86_64 

All errors (new ones prefixed by >>):

   drivers/md/dm-ioctl.c: In function 'is_valid_type':
>> drivers/md/dm-ioctl.c:1273:42: error: 'DM_TYPE_DAX_BIO_BASED' undeclared (first use in this function)
         (cur == DM_TYPE_BIO_BASED && new == DM_TYPE_DAX_BIO_BASED))
                                             ^~~~~~~~~~~~~~~~~~~~~
   drivers/md/dm-ioctl.c:1273:42: note: each undeclared identifier is reported only once for each function it appears in

vim +/DM_TYPE_DAX_BIO_BASED +1273 drivers/md/dm-ioctl.c

  1267		return dm_table_complete(table);
  1268	}
  1269	
  1270	static bool is_valid_type(unsigned cur, unsigned new)
  1271	{
  1272		if (cur == new ||
> 1273		    (cur == DM_TYPE_BIO_BASED && new == DM_TYPE_DAX_BIO_BASED))
  1274			return true;
  1275	
  1276		return false;

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/octet-stream, Size: 24468 bytes --]

[PATCH 2/2] dm snap: add fake origin_direct_access

[Toshi Kani]

dax-capable mapped-device is marked as DM_TYPE_DAX_BIO_BASED,
which supports both dax and bio-based operations.  dm-snap
needs to work with dax-capable device when bio-based operation
is used.

Add fake origin_direct_access() to origin device so that its
origin device is also marked as DM_TYPE_DAX_BIO_BASED for
dax-capable device.  This allows to extend target's DM table.
dm-snap works normally when bio-based operation is used.

dm-snap does not support dax operation, and mount with dax
option to a target device or snapshot device fails.

Signed-off-by: Toshi Kani <toshi.kani@hpe.com>
Cc: Mike Snitzer <snitzer@redhat.com>
Cc: Alasdair Kergon <agk@redhat.com>
Cc: Dan Williams <dan.j.williams@intel.com>
---
 drivers/md/dm-snap.c |    8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/drivers/md/dm-snap.c b/drivers/md/dm-snap.c
index 69ab1ff..c472f04 100644
--- a/drivers/md/dm-snap.c
+++ b/drivers/md/dm-snap.c
@@ -2301,6 +2301,13 @@ static int origin_map(struct dm_target *ti, struct bio *bio)
 	return do_origin(o->dev, bio);
 }
 
+static long origin_direct_access(struct dm_target *ti, sector_t sector,
+		void __pmem **kaddr, pfn_t *pfn, long size)
+{
+	DMWARN("device does not support dax.");
+	return -EIO;
+}
+
 /*
  * Set the target "max_io_len" field to the minimum of all the snapshots'
  * chunk sizes.
@@ -2360,6 +2367,7 @@ static struct target_type origin_target = {
 	.postsuspend = origin_postsuspend,
 	.status  = origin_status,
 	.iterate_devices = origin_iterate_devices,
+	.direct_access = origin_direct_access,
 };
 
 static struct target_type snapshot_target = {

Re: [PATCH 2/2] dm snap: add fake origin_direct_access

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 1951 bytes --]

Hi,

[auto build test WARNING on dm/for-next]
[also build test WARNING on v4.7-rc5 next-20160628]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Toshi-Kani/Support-DAX-for-device-mapper-dm-linear-devices/20160629-034110
base:   https://git.kernel.org/pub/scm/linux/kernel/git/device-mapper/linux-dm.git for-next
config: x86_64-lkp (attached as .config)
compiler: gcc-4.9 (Debian 4.9.3-14) 4.9.3
reproduce:
        # save the attached .config to linux build tree
        make ARCH=x86_64 

All warnings (new ones prefixed by >>):

   drivers/md/dm-snap.c:2370:2: error: unknown field 'direct_access' specified in initializer
     .direct_access = origin_direct_access,
     ^
>> drivers/md/dm-snap.c:2370:2: warning: initialization from incompatible pointer type
   drivers/md/dm-snap.c:2370:2: warning: (near initialization for 'origin_target.io_hints')

vim +2370 drivers/md/dm-snap.c

  2354		struct dm_origin *o = ti->private;
  2355	
  2356		return fn(ti, o->dev, 0, ti->len, data);
  2357	}
  2358	
  2359	static struct target_type origin_target = {
  2360		.name    = "snapshot-origin",
  2361		.version = {1, 9, 0},
  2362		.module  = THIS_MODULE,
  2363		.ctr     = origin_ctr,
  2364		.dtr     = origin_dtr,
  2365		.map     = origin_map,
  2366		.resume  = origin_resume,
  2367		.postsuspend = origin_postsuspend,
  2368		.status  = origin_status,
  2369		.iterate_devices = origin_iterate_devices,
> 2370		.direct_access = origin_direct_access,
  2371	};
  2372	
  2373	static struct target_type snapshot_target = {
  2374		.name    = "snapshot",
  2375		.version = {1, 15, 0},
  2376		.module  = THIS_MODULE,
  2377		.ctr     = snapshot_ctr,
  2378		.dtr     = snapshot_dtr,

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/octet-stream, Size: 23378 bytes --]

Re: [PATCH 0/2] fix dm-snap for dax

[Kani, Toshimitsu]

Sorry, subject line should be "[PATCH 0/2] fix dm-snap for dax"
-Toshi

On Tue, 2016-06-28 at 13:37 -0600, Toshi Kani wrote:
> I noticed that dm-snap reloads DM table of target mapped-device, which
> fails for dax-capable device after dax support is added.  Ideally,
> adding dax support to dm-snap solves the issue, but it cannot be done
> easily.  This patch-set allows dm-snap to work with dax-capable target
> devices when bio-based operation is used.
> 
> dax operation is unsupported with dm-snap, such that:
>  a) After snapshot is taken, mount with dax option to a target device
>     or a snapshot device fails.  They can be mounted without dax.
>  b) After snapshot is taken to a dax-mounted target device, any writes 
>     to the target device fails (EIO).
> 
> b) can be protected by changing lvcreate to fail when snapshot is
> requested to a dax-mounted target device. 
> 
> - Patch 1 solves an error when lvremove is made to a snapshot device.
> - Patch 2 solves an error when lvcreate --snapshot is made to a dax-
>   capable device.
> 
> ---
> Toshi Kani (2):
>  1/2 dm: update table type check for dax
>  2/2 dm snap: add fake origin_direct_access
> 
> ---
>  drivers/md/dm-ioctl.c | 11 ++++++++++-
>  drivers/md/dm-snap.c  |  8 ++++++++
>  2 files changed, 18 insertions(+), 1 deletion(-)
> _______________________________________________
> Linux-nvdimm mailing list
> Linux-nvdimm@lists.01.org
> https://lists.01.org/mailman/listinfo/linux-nvdimm