Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1433242 > unrolled thread

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

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [PATCH 1/2] dm: update table type check for dax Toshi Kani <toshi.kani@hpe.com> - 2016-06-28 21:40 +0200
    Re: [PATCH 1/2] dm: update table type check for dax Mike Snitzer <snitzer@redhat.com> - 2016-06-28 22:10 +0200
      Re: [PATCH 1/2] dm: update table type check for dax "Kani, Toshimitsu" <toshi.kani@hpe.com> - 2016-06-28 22:30 +0200
        Re: [PATCH 1/2] dm: update table type check for dax Mike Snitzer <snitzer@redhat.com> - 2016-06-29 02:50 +0200
          Re: [PATCH 1/2] dm: update table type check for dax "Kani, Toshimitsu" <toshi.kani@hpe.com> - 2016-06-29 17:20 +0200
    Re: [PATCH 1/2] dm: update table type check for dax kbuild test robot <lkp@intel.com> - 2016-06-28 22:40 +0200

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

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;

[toc] | [next] | [standalone]

#1433263

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.

[toc] | [prev] | [next] | [standalone]

#1433277

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

[toc] | [prev] | [next] | [standalone]

#1433385

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.

[toc] | [prev] | [next] | [standalone]

#1433844

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

[toc] | [prev] | [next] | [standalone]

#1433286

[Multipart message — attachments visible in raw view] — view raw

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

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web