| Message ID | 1467142636-21094-2-git-send-email-toshi.kani@hpe.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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.
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
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
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.
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
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;
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(-)