[PATCH v3 0/4] introduce interface to list badblocks

[PATCH v3 0/4] introduce interface to list badblocks

[yangerkun]

Since .message support report results to user directly. We can change
some type of message(queryblock/countbadblocks/removebadblock) to return
results to user.

Besides, we add a interface 'listbadblocks' to list all bad block. If
no bad block exists, we will return 'null', or we will list them multi
line which each line means one bad block.

v2->v3:
Realize this logical in .message, change logical for some type message to
report results to user too.

yangerkun (4):
  dm dust: report some message results back to user directly
  dm dust: update doc after message results report to user directly
  dm dust: add interface to list all badblocks
  dm dust: introduce listbadblocks in the rst

 .../admin-guide/device-mapper/dm-dust.rst     | 31 +++++++---
 drivers/md/dm-dust.c                          | 58 ++++++++++++++-----
 2 files changed, 69 insertions(+), 20 deletions(-)

-- 
2.25.4

[PATCH v3 1/4] dm dust: report some message results back to user directly

[yangerkun]

Some type of message(queryblock/countbadblocks/removebadblock) may better
report results to user directly. Do it with DMEMIT.

Signed-off-by: yangerkun <yangerkun@huawei.com>
---
 drivers/md/dm-dust.c | 31 ++++++++++++++++++-------------
 1 file changed, 18 insertions(+), 13 deletions(-)

diff --git a/drivers/md/dm-dust.c b/drivers/md/dm-dust.c
index ff03b90072c5..a0c75c104de0 100644
--- a/drivers/md/dm-dust.c
+++ b/drivers/md/dm-dust.c
@@ -138,20 +138,22 @@ static int dust_add_block(struct dust_device *dd, unsigned long long block,
 	return 0;
 }
 
-static int dust_query_block(struct dust_device *dd, unsigned long long block)
+static int dust_query_block(struct dust_device *dd, unsigned long long block, char *result,
+			    unsigned int maxlen, unsigned int *sz_ptr)
 {
 	struct badblock *bblock;
 	unsigned long flags;
+	unsigned int sz = *sz_ptr;
 
 	spin_lock_irqsave(&dd->dust_lock, flags);
 	bblock = dust_rb_search(&dd->badblocklist, block);
 	if (bblock != NULL)
-		DMINFO("%s: block %llu found in badblocklist", __func__, block);
+		DMEMIT("block %llu found in badblocklist", block);
 	else
-		DMINFO("%s: block %llu not found in badblocklist", __func__, block);
+		DMEMIT("block %llu not found in badblocklist", block);
 	spin_unlock_irqrestore(&dd->dust_lock, flags);
 
-	return 0;
+	return 1;
 }
 
 static int __dust_map_read(struct dust_device *dd, sector_t thisblock)
@@ -259,11 +261,13 @@ static bool __dust_clear_badblocks(struct rb_root *tree,
 	return true;
 }
 
-static int dust_clear_badblocks(struct dust_device *dd)
+static int dust_clear_badblocks(struct dust_device *dd, char *result, unsigned int maxlen,
+				unsigned int *sz_ptr)
 {
 	unsigned long flags;
 	struct rb_root badblocklist;
 	unsigned long long badblock_count;
+	unsigned int sz = *sz_ptr;
 
 	spin_lock_irqsave(&dd->dust_lock, flags);
 	badblocklist = dd->badblocklist;
@@ -273,11 +277,11 @@ static int dust_clear_badblocks(struct dust_device *dd)
 	spin_unlock_irqrestore(&dd->dust_lock, flags);
 
 	if (!__dust_clear_badblocks(&badblocklist, badblock_count))
-		DMINFO("%s: no badblocks found", __func__);
+		DMEMIT("no badblocks found");
 	else
-		DMINFO("%s: badblocks cleared", __func__);
+		DMEMIT("badblocks cleared");
 
-	return 0;
+	return 1;
 }
 
 /*
@@ -383,7 +387,7 @@ static void dust_dtr(struct dm_target *ti)
 }
 
 static int dust_message(struct dm_target *ti, unsigned int argc, char **argv,
-			char *result_buf, unsigned int maxlen)
+			char *result, unsigned int maxlen)
 {
 	struct dust_device *dd = ti->private;
 	sector_t size = i_size_read(dd->dev->bdev->bd_inode) >> SECTOR_SHIFT;
@@ -393,6 +397,7 @@ static int dust_message(struct dm_target *ti, unsigned int argc, char **argv,
 	unsigned char wr_fail_cnt;
 	unsigned int tmp_ui;
 	unsigned long flags;
+	unsigned int sz = 0;
 	char dummy;
 
 	if (argc == 1) {
@@ -410,12 +415,12 @@ static int dust_message(struct dm_target *ti, unsigned int argc, char **argv,
 			r = 0;
 		} else if (!strcasecmp(argv[0], "countbadblocks")) {
 			spin_lock_irqsave(&dd->dust_lock, flags);
-			DMINFO("countbadblocks: %llu badblock(s) found",
+			DMEMIT("countbadblocks: %llu badblock(s) found",
 			       dd->badblock_count);
 			spin_unlock_irqrestore(&dd->dust_lock, flags);
-			r = 0;
+			r = 1;
 		} else if (!strcasecmp(argv[0], "clearbadblocks")) {
-			r = dust_clear_badblocks(dd);
+			r = dust_clear_badblocks(dd, result, maxlen, &sz);
 		} else if (!strcasecmp(argv[0], "quiet")) {
 			if (!dd->quiet_mode)
 				dd->quiet_mode = true;
@@ -441,7 +446,7 @@ static int dust_message(struct dm_target *ti, unsigned int argc, char **argv,
 		else if (!strcasecmp(argv[0], "removebadblock"))
 			r = dust_remove_block(dd, block);
 		else if (!strcasecmp(argv[0], "queryblock"))
-			r = dust_query_block(dd, block);
+			r = dust_query_block(dd, block, result, maxlen, &sz);
 		else
 			invalid_msg = true;
 
-- 
2.25.4

[PATCH v3 2/4] dm dust: update doc after message results report to user directly

[yangerkun]

Since some type of message will report the results to user directly,
we should update the doc too.

Signed-off-by: yangerkun <yangerkun@huawei.com>
---
 .../admin-guide/device-mapper/dm-dust.rst         | 15 ++++++++-------
 1 file changed, 8 insertions(+), 7 deletions(-)

diff --git a/Documentation/admin-guide/device-mapper/dm-dust.rst b/Documentation/admin-guide/device-mapper/dm-dust.rst
index b6e7e7ead831..84149c08b68f 100644
--- a/Documentation/admin-guide/device-mapper/dm-dust.rst
+++ b/Documentation/admin-guide/device-mapper/dm-dust.rst
@@ -69,10 +69,11 @@ Create the dm-dust device:
         $ sudo dmsetup create dust1 --table '0 33552384 dust /dev/vdb1 0 4096'
 
 Check the status of the read behavior ("bypass" indicates that all I/O
-will be passed through to the underlying device)::
+will be passed through to the underlying device, "verbose" indicates that
+we will log everythings like bad blocks added, removed, or "remapped")::
 
         $ sudo dmsetup status dust1
-        0 33552384 dust 252:17 bypass
+        0 33552384 dust 252:17 bypass verbose
 
         $ sudo dd if=/dev/mapper/dust1 of=/dev/null bs=512 count=128 iflag=direct
         128+0 records in
@@ -164,7 +165,7 @@ following message command::
 A message will print with the number of bad blocks currently
 configured on the device::
 
-        kernel: device-mapper: dust: countbadblocks: 895 badblock(s) found
+        countbadblocks: 895 badblock(s) found
 
 Querying for specific bad blocks
 --------------------------------
@@ -176,11 +177,11 @@ following message command::
 
 The following message will print if the block is in the list::
 
-        device-mapper: dust: queryblock: block 72 found in badblocklist
+        block 72 found in badblocklist
 
 The following message will print if the block is not in the list::
 
-        device-mapper: dust: queryblock: block 72 not found in badblocklist
+        block 72 not found in badblocklist
 
 The "queryblock" message command will work in both the "enabled"
 and "disabled" modes, allowing the verification of whether a block
@@ -198,12 +199,12 @@ following message command::
 
 After clearing the bad block list, the following message will appear::
 
-        kernel: device-mapper: dust: clearbadblocks: badblocks cleared
+        badblocks cleared
 
 If there were no bad blocks to clear, the following message will
 appear::
 
-        kernel: device-mapper: dust: clearbadblocks: no badblocks found
+        no badblocks found
 
 Message commands list
 ---------------------
-- 
2.25.4

[PATCH v3 3/4] dm dust: add interface to list all badblocks

[yangerkun]

This interface may help anyone who want to know all badblocks without
query block for each.

Signed-off-by: yangerkun <yangerkun@huawei.com>
---
 drivers/md/dm-dust.c | 27 +++++++++++++++++++++++++++
 1 file changed, 27 insertions(+)

diff --git a/drivers/md/dm-dust.c b/drivers/md/dm-dust.c
index a0c75c104de0..2ad8fc9293e0 100644
--- a/drivers/md/dm-dust.c
+++ b/drivers/md/dm-dust.c
@@ -284,6 +284,31 @@ static int dust_clear_badblocks(struct dust_device *dd, char *result, unsigned i
 	return 1;
 }
 
+static int dust_list_badblocks(struct dust_device *dd, char *result, unsigned int maxlen,
+				unsigned int *sz_ptr)
+{
+	unsigned long flags;
+	struct rb_root badblocklist;
+	struct rb_node *node;
+	struct badblock *bblk;
+	unsigned int sz = *sz_ptr;
+	unsigned long long num = 0;
+
+	spin_lock_irqsave(&dd->dust_lock, flags);
+	badblocklist = dd->badblocklist;
+	for (node = rb_first(&badblocklist); node; node = rb_next(node)) {
+		bblk = rb_entry(node, struct badblock, node);
+		DMEMIT("%llu\n", bblk->bb);
+		num++;
+	}
+
+	spin_unlock_irqrestore(&dd->dust_lock, flags);
+	if (!num)
+		DMEMIT("null");
+
+	return 1;
+}
+
 /*
  * Target parameters:
  *
@@ -427,6 +452,8 @@ static int dust_message(struct dm_target *ti, unsigned int argc, char **argv,
 			else
 				dd->quiet_mode = false;
 			r = 0;
+		} else if (!strcasecmp(argv[0], "listbadblocks")) {
+			r = dust_list_badblocks(dd, result, maxlen, &sz);
 		} else {
 			invalid_msg = true;
 		}
-- 
2.25.4

[PATCH v3 4/4] dm dust: introduce listbadblocks in the rst

[yangerkun]

Since we support the listbadblocks command, introduce the detail in the
doc.

Signed-off-by: yangerkun <yangerkun@huawei.com>
---
 .../admin-guide/device-mapper/dm-dust.rst        | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/Documentation/admin-guide/device-mapper/dm-dust.rst b/Documentation/admin-guide/device-mapper/dm-dust.rst
index 84149c08b68f..5c66a71e3442 100644
--- a/Documentation/admin-guide/device-mapper/dm-dust.rst
+++ b/Documentation/admin-guide/device-mapper/dm-dust.rst
@@ -205,6 +205,21 @@ If there were no bad blocks to clear, the following message will
 appear::
 
         no badblocks found
+Listing the bad block list
+--------------------------
+
+To list all bad blocks in the bad block list(using an example device
+with blocks 1 and 2 in the bad block list),run the following message
+command::
+
+        $ sudo dmsetup message dust1 0 listbadblocks
+        1
+        2
+
+No bad block exists, same message will get following report::
+
+        $ sudo dmsetup message dust1 0 listbadblocks
+        null
 
 Message commands list
 ---------------------
@@ -224,6 +239,7 @@ Single argument message commands::
 
         countbadblocks
         clearbadblocks
+        listbadblocks
         disable
         enable
         quiet
-- 
2.25.4

Re: [PATCH v3 1/4] dm dust: report some message results back to user directly

[Bryan Gurney]

On Fri, Jun 19, 2020 at 8:37 AM yangerkun <yangerkun@huawei.com> wrote:
>
> Some type of message(queryblock/countbadblocks/removebadblock) may better
> report results to user directly. Do it with DMEMIT.
>
> Signed-off-by: yangerkun <yangerkun@huawei.com>
> ---
>  drivers/md/dm-dust.c | 31 ++++++++++++++++++-------------
>  1 file changed, 18 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/md/dm-dust.c b/drivers/md/dm-dust.c
> index ff03b90072c5..a0c75c104de0 100644
> --- a/drivers/md/dm-dust.c
> +++ b/drivers/md/dm-dust.c
> @@ -138,20 +138,22 @@ static int dust_add_block(struct dust_device *dd, unsigned long long block,
>         return 0;
>  }
>
> -static int dust_query_block(struct dust_device *dd, unsigned long long block)
> +static int dust_query_block(struct dust_device *dd, unsigned long long block, char *result,
> +                           unsigned int maxlen, unsigned int *sz_ptr)
>  {
>         struct badblock *bblock;
>         unsigned long flags;
> +       unsigned int sz = *sz_ptr;
>
>         spin_lock_irqsave(&dd->dust_lock, flags);
>         bblock = dust_rb_search(&dd->badblocklist, block);
>         if (bblock != NULL)
> -               DMINFO("%s: block %llu found in badblocklist", __func__, block);
> +               DMEMIT("block %llu found in badblocklist", block);
>         else
> -               DMINFO("%s: block %llu not found in badblocklist", __func__, block);
> +               DMEMIT("block %llu not found in badblocklist", block);
>         spin_unlock_irqrestore(&dd->dust_lock, flags);
>
> -       return 0;
> +       return 1;

First, thank you very much for this patch.  After the concerns to
convert some functions to use DMEMIT were brought up, I was trying to
start the conversion, when this patch arrived, so I installed it, and
tested it.

I do have a question, though:

First, I see that in dust_query_block() (above) and
dust_clear_badblocks(), the "return 0" statement is changed to "return
1".

(Additionally, there is a change from "r = 0" to  "r = 1", in the
"countbadblocks" message handler)

On testing the functions, they still work, but why was this change
made?  Is it related to the use of DMEMIT?


Thanks,

Bryan

>  }
>
>  static int __dust_map_read(struct dust_device *dd, sector_t thisblock)
> @@ -259,11 +261,13 @@ static bool __dust_clear_badblocks(struct rb_root *tree,
>         return true;
>  }
>
> -static int dust_clear_badblocks(struct dust_device *dd)
> +static int dust_clear_badblocks(struct dust_device *dd, char *result, unsigned int maxlen,
> +                               unsigned int *sz_ptr)
>  {
>         unsigned long flags;
>         struct rb_root badblocklist;
>         unsigned long long badblock_count;
> +       unsigned int sz = *sz_ptr;
>
>         spin_lock_irqsave(&dd->dust_lock, flags);
>         badblocklist = dd->badblocklist;
> @@ -273,11 +277,11 @@ static int dust_clear_badblocks(struct dust_device *dd)
>         spin_unlock_irqrestore(&dd->dust_lock, flags);
>
>         if (!__dust_clear_badblocks(&badblocklist, badblock_count))
> -               DMINFO("%s: no badblocks found", __func__);
> +               DMEMIT("no badblocks found");
>         else
> -               DMINFO("%s: badblocks cleared", __func__);
> +               DMEMIT("badblocks cleared");
>
> -       return 0;
> +       return 1;
>  }
>
>  /*
> @@ -383,7 +387,7 @@ static void dust_dtr(struct dm_target *ti)
>  }
>
>  static int dust_message(struct dm_target *ti, unsigned int argc, char **argv,
> -                       char *result_buf, unsigned int maxlen)
> +                       char *result, unsigned int maxlen)
>  {
>         struct dust_device *dd = ti->private;
>         sector_t size = i_size_read(dd->dev->bdev->bd_inode) >> SECTOR_SHIFT;
> @@ -393,6 +397,7 @@ static int dust_message(struct dm_target *ti, unsigned int argc, char **argv,
>         unsigned char wr_fail_cnt;
>         unsigned int tmp_ui;
>         unsigned long flags;
> +       unsigned int sz = 0;
>         char dummy;
>
>         if (argc == 1) {
> @@ -410,12 +415,12 @@ static int dust_message(struct dm_target *ti, unsigned int argc, char **argv,
>                         r = 0;
>                 } else if (!strcasecmp(argv[0], "countbadblocks")) {
>                         spin_lock_irqsave(&dd->dust_lock, flags);
> -                       DMINFO("countbadblocks: %llu badblock(s) found",
> +                       DMEMIT("countbadblocks: %llu badblock(s) found",
>                                dd->badblock_count);
>                         spin_unlock_irqrestore(&dd->dust_lock, flags);
> -                       r = 0;
> +                       r = 1;
>                 } else if (!strcasecmp(argv[0], "clearbadblocks")) {
> -                       r = dust_clear_badblocks(dd);
> +                       r = dust_clear_badblocks(dd, result, maxlen, &sz);
>                 } else if (!strcasecmp(argv[0], "quiet")) {
>                         if (!dd->quiet_mode)
>                                 dd->quiet_mode = true;
> @@ -441,7 +446,7 @@ static int dust_message(struct dm_target *ti, unsigned int argc, char **argv,
>                 else if (!strcasecmp(argv[0], "removebadblock"))
>                         r = dust_remove_block(dd, block);
>                 else if (!strcasecmp(argv[0], "queryblock"))
> -                       r = dust_query_block(dd, block);
> +                       r = dust_query_block(dd, block, result, maxlen, &sz);
>                 else
>                         invalid_msg = true;
>
> --
> 2.25.4
>

Re: [PATCH v3 1/4] dm dust: report some message results back to user directly

[Mike Snitzer]

On Fri, Jun 19 2020 at 11:40am -0400,
Bryan Gurney <bgurney@redhat.com> wrote:

> On Fri, Jun 19, 2020 at 8:37 AM yangerkun <yangerkun@huawei.com> wrote:
> >
> > Some type of message(queryblock/countbadblocks/removebadblock) may better
> > report results to user directly. Do it with DMEMIT.
> >
> > Signed-off-by: yangerkun <yangerkun@huawei.com>
> > ---
> >  drivers/md/dm-dust.c | 31 ++++++++++++++++++-------------
> >  1 file changed, 18 insertions(+), 13 deletions(-)
> >
> > diff --git a/drivers/md/dm-dust.c b/drivers/md/dm-dust.c
> > index ff03b90072c5..a0c75c104de0 100644
> > --- a/drivers/md/dm-dust.c
> > +++ b/drivers/md/dm-dust.c
> > @@ -138,20 +138,22 @@ static int dust_add_block(struct dust_device *dd, unsigned long long block,
> >         return 0;
> >  }
> >
> > -static int dust_query_block(struct dust_device *dd, unsigned long long block)
> > +static int dust_query_block(struct dust_device *dd, unsigned long long block, char *result,
> > +                           unsigned int maxlen, unsigned int *sz_ptr)
> >  {
> >         struct badblock *bblock;
> >         unsigned long flags;
> > +       unsigned int sz = *sz_ptr;
> >
> >         spin_lock_irqsave(&dd->dust_lock, flags);
> >         bblock = dust_rb_search(&dd->badblocklist, block);
> >         if (bblock != NULL)
> > -               DMINFO("%s: block %llu found in badblocklist", __func__, block);
> > +               DMEMIT("block %llu found in badblocklist", block);
> >         else
> > -               DMINFO("%s: block %llu not found in badblocklist", __func__, block);
> > +               DMEMIT("block %llu not found in badblocklist", block);
> >         spin_unlock_irqrestore(&dd->dust_lock, flags);
> >
> > -       return 0;
> > +       return 1;
> 
> First, thank you very much for this patch.  After the concerns to
> convert some functions to use DMEMIT were brought up, I was trying to
> start the conversion, when this patch arrived, so I installed it, and
> tested it.
> 
> I do have a question, though:
> 
> First, I see that in dust_query_block() (above) and
> dust_clear_badblocks(), the "return 0" statement is changed to "return
> 1".
> 
> (Additionally, there is a change from "r = 0" to  "r = 1", in the
> "countbadblocks" message handler)
> 
> On testing the functions, they still work, but why was this change
> made?  Is it related to the use of DMEMIT?

It is, but we need to review the returns closer.  Looked to me that 1
was being returned even if nothing was DMEMIT()'d.. but I could be wrong
(only looked quickly).

I also noticed that some output was changed to not include __func__.
Please review that the output reflects what you'd like displayed.

Mike

Re: [PATCH v3 2/4] dm dust: update doc after message results report to user directly

[Bryan Gurney]

On Fri, Jun 19, 2020 at 8:37 AM yangerkun <yangerkun@huawei.com> wrote:
>
> Since some type of message will report the results to user directly,
> we should update the doc too.
>
> Signed-off-by: yangerkun <yangerkun@huawei.com>
> ---
>  .../admin-guide/device-mapper/dm-dust.rst         | 15 ++++++++-------
>  1 file changed, 8 insertions(+), 7 deletions(-)
>
> diff --git a/Documentation/admin-guide/device-mapper/dm-dust.rst b/Documentation/admin-guide/device-mapper/dm-dust.rst
> index b6e7e7ead831..84149c08b68f 100644
> --- a/Documentation/admin-guide/device-mapper/dm-dust.rst
> +++ b/Documentation/admin-guide/device-mapper/dm-dust.rst
> @@ -69,10 +69,11 @@ Create the dm-dust device:
>          $ sudo dmsetup create dust1 --table '0 33552384 dust /dev/vdb1 0 4096'
>
>  Check the status of the read behavior ("bypass" indicates that all I/O
> -will be passed through to the underlying device)::
> +will be passed through to the underlying device, "verbose" indicates that
> +we will log everythings like bad blocks added, removed, or "remapped")::

Thanks for the addition here, and the revision of the "dmsetup status"
output example below.  Here's a quick grammatical fix:

'..."verbose" indicates that bad block additions, removals, and remaps
will be verbosely logged'

After that change,

Reviewed-by: Bryan Gurney <bgurney@redhat.com>


Thanks,

Bryan

>
>          $ sudo dmsetup status dust1
> -        0 33552384 dust 252:17 bypass
> +        0 33552384 dust 252:17 bypass verbose
>
>          $ sudo dd if=/dev/mapper/dust1 of=/dev/null bs=512 count=128 iflag=direct
>          128+0 records in
> @@ -164,7 +165,7 @@ following message command::
>  A message will print with the number of bad blocks currently
>  configured on the device::
>
> -        kernel: device-mapper: dust: countbadblocks: 895 badblock(s) found
> +        countbadblocks: 895 badblock(s) found
>
>  Querying for specific bad blocks
>  --------------------------------
> @@ -176,11 +177,11 @@ following message command::
>
>  The following message will print if the block is in the list::
>
> -        device-mapper: dust: queryblock: block 72 found in badblocklist
> +        block 72 found in badblocklist
>
>  The following message will print if the block is not in the list::
>
> -        device-mapper: dust: queryblock: block 72 not found in badblocklist
> +        block 72 not found in badblocklist
>
>  The "queryblock" message command will work in both the "enabled"
>  and "disabled" modes, allowing the verification of whether a block
> @@ -198,12 +199,12 @@ following message command::
>
>  After clearing the bad block list, the following message will appear::
>
> -        kernel: device-mapper: dust: clearbadblocks: badblocks cleared
> +        badblocks cleared
>
>  If there were no bad blocks to clear, the following message will
>  appear::
>
> -        kernel: device-mapper: dust: clearbadblocks: no badblocks found
> +        no badblocks found
>
>  Message commands list
>  ---------------------
> --
> 2.25.4
>

Re: [PATCH v3 3/4] dm dust: add interface to list all badblocks

[Bryan Gurney]

On Fri, Jun 19, 2020 at 8:37 AM yangerkun <yangerkun@huawei.com> wrote:
>
> This interface may help anyone who want to know all badblocks without
> query block for each.
>
> Signed-off-by: yangerkun <yangerkun@huawei.com>
> ---
>  drivers/md/dm-dust.c | 27 +++++++++++++++++++++++++++
>  1 file changed, 27 insertions(+)
>
> diff --git a/drivers/md/dm-dust.c b/drivers/md/dm-dust.c
> index a0c75c104de0..2ad8fc9293e0 100644
> --- a/drivers/md/dm-dust.c
> +++ b/drivers/md/dm-dust.c
> @@ -284,6 +284,31 @@ static int dust_clear_badblocks(struct dust_device *dd, char *result, unsigned i
>         return 1;
>  }
>
> +static int dust_list_badblocks(struct dust_device *dd, char *result, unsigned int maxlen,
> +                               unsigned int *sz_ptr)
> +{
> +       unsigned long flags;
> +       struct rb_root badblocklist;
> +       struct rb_node *node;
> +       struct badblock *bblk;
> +       unsigned int sz = *sz_ptr;
> +       unsigned long long num = 0;
> +
> +       spin_lock_irqsave(&dd->dust_lock, flags);
> +       badblocklist = dd->badblocklist;
> +       for (node = rb_first(&badblocklist); node; node = rb_next(node)) {
> +               bblk = rb_entry(node, struct badblock, node);
> +               DMEMIT("%llu\n", bblk->bb);
> +               num++;
> +       }
> +
> +       spin_unlock_irqrestore(&dd->dust_lock, flags);
> +       if (!num)
> +               DMEMIT("null");

Simply printing "null" may confuse the user into thinking that
something is wrong, when in fact everything is running normally, and
the bad block list is empty.

Heinz recommended returning an empty report, and I agree with that.
To the user, it would look something like this:

$ sudo dmsetup message dust1 0 listbadblocks

$ sudo dmsetup message dust1 0 countbadblocks
countbadblocks: 0 badblock(s) found


Thanks,

Bryan

> +
> +       return 1;
> +}
> +
>  /*
>   * Target parameters:
>   *
> @@ -427,6 +452,8 @@ static int dust_message(struct dm_target *ti, unsigned int argc, char **argv,
>                         else
>                                 dd->quiet_mode = false;
>                         r = 0;
> +               } else if (!strcasecmp(argv[0], "listbadblocks")) {
> +                       r = dust_list_badblocks(dd, result, maxlen, &sz);
>                 } else {
>                         invalid_msg = true;
>                 }
> --
> 2.25.4
>

Re: [PATCH v3 1/4] dm dust: report some message results back to user directly

[Bryan Gurney]

On Fri, Jun 19, 2020 at 11:45 AM Mike Snitzer <snitzer@redhat.com> wrote:
>
> On Fri, Jun 19 2020 at 11:40am -0400,
> Bryan Gurney <bgurney@redhat.com> wrote:
>
> > On Fri, Jun 19, 2020 at 8:37 AM yangerkun <yangerkun@huawei.com> wrote:
> > >
> > > Some type of message(queryblock/countbadblocks/removebadblock) may better
> > > report results to user directly. Do it with DMEMIT.
> > >
> > > Signed-off-by: yangerkun <yangerkun@huawei.com>
> > > ---
> > >  drivers/md/dm-dust.c | 31 ++++++++++++++++++-------------
> > >  1 file changed, 18 insertions(+), 13 deletions(-)
> > >
> > > diff --git a/drivers/md/dm-dust.c b/drivers/md/dm-dust.c
> > > index ff03b90072c5..a0c75c104de0 100644
> > > --- a/drivers/md/dm-dust.c
> > > +++ b/drivers/md/dm-dust.c
> > > @@ -138,20 +138,22 @@ static int dust_add_block(struct dust_device *dd, unsigned long long block,
> > >         return 0;
> > >  }
> > >
> > > -static int dust_query_block(struct dust_device *dd, unsigned long long block)
> > > +static int dust_query_block(struct dust_device *dd, unsigned long long block, char *result,
> > > +                           unsigned int maxlen, unsigned int *sz_ptr)
> > >  {
> > >         struct badblock *bblock;
> > >         unsigned long flags;
> > > +       unsigned int sz = *sz_ptr;
> > >
> > >         spin_lock_irqsave(&dd->dust_lock, flags);
> > >         bblock = dust_rb_search(&dd->badblocklist, block);
> > >         if (bblock != NULL)
> > > -               DMINFO("%s: block %llu found in badblocklist", __func__, block);
> > > +               DMEMIT("block %llu found in badblocklist", block);
> > >         else
> > > -               DMINFO("%s: block %llu not found in badblocklist", __func__, block);
> > > +               DMEMIT("block %llu not found in badblocklist", block);
> > >         spin_unlock_irqrestore(&dd->dust_lock, flags);
> > >
> > > -       return 0;
> > > +       return 1;
> >
> > First, thank you very much for this patch.  After the concerns to
> > convert some functions to use DMEMIT were brought up, I was trying to
> > start the conversion, when this patch arrived, so I installed it, and
> > tested it.
> >
> > I do have a question, though:
> >
> > First, I see that in dust_query_block() (above) and
> > dust_clear_badblocks(), the "return 0" statement is changed to "return
> > 1".
> >
> > (Additionally, there is a change from "r = 0" to  "r = 1", in the
> > "countbadblocks" message handler)
> >
> > On testing the functions, they still work, but why was this change
> > made?  Is it related to the use of DMEMIT?
>
> It is, but we need to review the returns closer.  Looked to me that 1
> was being returned even if nothing was DMEMIT()'d.. but I could be wrong
> (only looked quickly).
>
> I also noticed that some output was changed to not include __func__.
> Please review that the output reflects what you'd like displayed.
>
> Mike
>

After adding __func__ back into the string, here's what it looks like:

$ sudo dmsetup message dust1 0 addbadblock 60
$ sudo dmsetup message dust1 0 queryblock 60
dust_query_block: block 60 found in badblocklist
$ sudo dmsetup message dust1 0 queryblock 61
dust_query_block: block 61 not found in badblocklist

I feel that the output's origin is more clear, so I'd like to leave
__func__ in the output.

I took a look at the DMEMIT calls, and in all three cases, it looks
like there's something DMEMIT()'d:

dust_query_block: either "block found", or "block not found".
dust_clear_badblocks: either "no badblocks found", or "badblocks cleared".
result of "message ... countbadblocks": always prints "%llu badblocks found".


Thanks,

Bryan

Re: [PATCH v3 1/4] dm dust: report some message results back to user directly

[Mike Snitzer]

On Fri, Jun 19 2020 at  1:23pm -0400,
Bryan Gurney <bgurney@redhat.com> wrote:

> On Fri, Jun 19, 2020 at 11:45 AM Mike Snitzer <snitzer@redhat.com> wrote:
> >
> > On Fri, Jun 19 2020 at 11:40am -0400,
> > Bryan Gurney <bgurney@redhat.com> wrote:
> >
> > > On Fri, Jun 19, 2020 at 8:37 AM yangerkun <yangerkun@huawei.com> wrote:
> > > >
> > > > Some type of message(queryblock/countbadblocks/removebadblock) may better
> > > > report results to user directly. Do it with DMEMIT.
> > > >
> > > > Signed-off-by: yangerkun <yangerkun@huawei.com>
> > > > ---
> > > >  drivers/md/dm-dust.c | 31 ++++++++++++++++++-------------
> > > >  1 file changed, 18 insertions(+), 13 deletions(-)
> > > >
> > > > diff --git a/drivers/md/dm-dust.c b/drivers/md/dm-dust.c
> > > > index ff03b90072c5..a0c75c104de0 100644
> > > > --- a/drivers/md/dm-dust.c
> > > > +++ b/drivers/md/dm-dust.c
> > > > @@ -138,20 +138,22 @@ static int dust_add_block(struct dust_device *dd, unsigned long long block,
> > > >         return 0;
> > > >  }
> > > >
> > > > -static int dust_query_block(struct dust_device *dd, unsigned long long block)
> > > > +static int dust_query_block(struct dust_device *dd, unsigned long long block, char *result,
> > > > +                           unsigned int maxlen, unsigned int *sz_ptr)
> > > >  {
> > > >         struct badblock *bblock;
> > > >         unsigned long flags;
> > > > +       unsigned int sz = *sz_ptr;
> > > >
> > > >         spin_lock_irqsave(&dd->dust_lock, flags);
> > > >         bblock = dust_rb_search(&dd->badblocklist, block);
> > > >         if (bblock != NULL)
> > > > -               DMINFO("%s: block %llu found in badblocklist", __func__, block);
> > > > +               DMEMIT("block %llu found in badblocklist", block);
> > > >         else
> > > > -               DMINFO("%s: block %llu not found in badblocklist", __func__, block);
> > > > +               DMEMIT("block %llu not found in badblocklist", block);
> > > >         spin_unlock_irqrestore(&dd->dust_lock, flags);
> > > >
> > > > -       return 0;
> > > > +       return 1;
> > >
> > > First, thank you very much for this patch.  After the concerns to
> > > convert some functions to use DMEMIT were brought up, I was trying to
> > > start the conversion, when this patch arrived, so I installed it, and
> > > tested it.
> > >
> > > I do have a question, though:
> > >
> > > First, I see that in dust_query_block() (above) and
> > > dust_clear_badblocks(), the "return 0" statement is changed to "return
> > > 1".
> > >
> > > (Additionally, there is a change from "r = 0" to  "r = 1", in the
> > > "countbadblocks" message handler)
> > >
> > > On testing the functions, they still work, but why was this change
> > > made?  Is it related to the use of DMEMIT?
> >
> > It is, but we need to review the returns closer.  Looked to me that 1
> > was being returned even if nothing was DMEMIT()'d.. but I could be wrong
> > (only looked quickly).
> >
> > I also noticed that some output was changed to not include __func__.
> > Please review that the output reflects what you'd like displayed.
> >
> > Mike
> >
> 
> After adding __func__ back into the string, here's what it looks like:
> 
> $ sudo dmsetup message dust1 0 addbadblock 60
> $ sudo dmsetup message dust1 0 queryblock 60
> dust_query_block: block 60 found in badblocklist
> $ sudo dmsetup message dust1 0 queryblock 61
> dust_query_block: block 61 not found in badblocklist
> 
> I feel that the output's origin is more clear, so I'd like to leave
> __func__ in the output.
> 
> I took a look at the DMEMIT calls, and in all three cases, it looks
> like there's something DMEMIT()'d:
> 
> dust_query_block: either "block found", or "block not found".
> dust_clear_badblocks: either "no badblocks found", or "badblocks cleared".
> result of "message ... countbadblocks": always prints "%llu badblocks found".

OK, can you provide an incremental patch that restores the __func__
where you'd like?  I can deal with it, but I'll be slower to do so.

Mike

Re: [PATCH v3 1/4] dm dust: report some message results back to user directly

[Bryan Gurney]

On Fri, Jun 19, 2020 at 1:29 PM Mike Snitzer <snitzer@redhat.com> wrote:
>
> On Fri, Jun 19 2020 at  1:23pm -0400,
> Bryan Gurney <bgurney@redhat.com> wrote:
>
> > On Fri, Jun 19, 2020 at 11:45 AM Mike Snitzer <snitzer@redhat.com> wrote:
> > >
> > > On Fri, Jun 19 2020 at 11:40am -0400,
> > > Bryan Gurney <bgurney@redhat.com> wrote:
> > >
> > > > On Fri, Jun 19, 2020 at 8:37 AM yangerkun <yangerkun@huawei.com> wrote:
> > > > >
> > > > > Some type of message(queryblock/countbadblocks/removebadblock) may better
> > > > > report results to user directly. Do it with DMEMIT.
> > > > >
> > > > > Signed-off-by: yangerkun <yangerkun@huawei.com>
> > > > > ---
> > > > >  drivers/md/dm-dust.c | 31 ++++++++++++++++++-------------
> > > > >  1 file changed, 18 insertions(+), 13 deletions(-)
> > > > >
> > > > > diff --git a/drivers/md/dm-dust.c b/drivers/md/dm-dust.c
> > > > > index ff03b90072c5..a0c75c104de0 100644
> > > > > --- a/drivers/md/dm-dust.c
> > > > > +++ b/drivers/md/dm-dust.c
> > > > > @@ -138,20 +138,22 @@ static int dust_add_block(struct dust_device *dd, unsigned long long block,
> > > > >         return 0;
> > > > >  }
> > > > >
> > > > > -static int dust_query_block(struct dust_device *dd, unsigned long long block)
> > > > > +static int dust_query_block(struct dust_device *dd, unsigned long long block, char *result,
> > > > > +                           unsigned int maxlen, unsigned int *sz_ptr)
> > > > >  {
> > > > >         struct badblock *bblock;
> > > > >         unsigned long flags;
> > > > > +       unsigned int sz = *sz_ptr;
> > > > >
> > > > >         spin_lock_irqsave(&dd->dust_lock, flags);
> > > > >         bblock = dust_rb_search(&dd->badblocklist, block);
> > > > >         if (bblock != NULL)
> > > > > -               DMINFO("%s: block %llu found in badblocklist", __func__, block);
> > > > > +               DMEMIT("block %llu found in badblocklist", block);
> > > > >         else
> > > > > -               DMINFO("%s: block %llu not found in badblocklist", __func__, block);
> > > > > +               DMEMIT("block %llu not found in badblocklist", block);
> > > > >         spin_unlock_irqrestore(&dd->dust_lock, flags);
> > > > >
> > > > > -       return 0;
> > > > > +       return 1;
> > > >
> > > > First, thank you very much for this patch.  After the concerns to
> > > > convert some functions to use DMEMIT were brought up, I was trying to
> > > > start the conversion, when this patch arrived, so I installed it, and
> > > > tested it.
> > > >
> > > > I do have a question, though:
> > > >
> > > > First, I see that in dust_query_block() (above) and
> > > > dust_clear_badblocks(), the "return 0" statement is changed to "return
> > > > 1".
> > > >
> > > > (Additionally, there is a change from "r = 0" to  "r = 1", in the
> > > > "countbadblocks" message handler)
> > > >
> > > > On testing the functions, they still work, but why was this change
> > > > made?  Is it related to the use of DMEMIT?
> > >
> > > It is, but we need to review the returns closer.  Looked to me that 1
> > > was being returned even if nothing was DMEMIT()'d.. but I could be wrong
> > > (only looked quickly).
> > >
> > > I also noticed that some output was changed to not include __func__.
> > > Please review that the output reflects what you'd like displayed.
> > >
> > > Mike
> > >
> >
> > After adding __func__ back into the string, here's what it looks like:
> >
> > $ sudo dmsetup message dust1 0 addbadblock 60
> > $ sudo dmsetup message dust1 0 queryblock 60
> > dust_query_block: block 60 found in badblocklist
> > $ sudo dmsetup message dust1 0 queryblock 61
> > dust_query_block: block 61 not found in badblocklist
> >
> > I feel that the output's origin is more clear, so I'd like to leave
> > __func__ in the output.
> >
> > I took a look at the DMEMIT calls, and in all three cases, it looks
> > like there's something DMEMIT()'d:
> >
> > dust_query_block: either "block found", or "block not found".
> > dust_clear_badblocks: either "no badblocks found", or "badblocks cleared".
> > result of "message ... countbadblocks": always prints "%llu badblocks found".
>
> OK, can you provide an incremental patch that restores the __func__
> where you'd like?  I can deal with it, but I'll be slower to do so.
>
> Mike
>

Yes, I just sent the patch to dm-devel.


Thanks,

Bryan

Re: [PATCH v3 4/4] dm dust: introduce listbadblocks in the rst

[Bryan Gurney]

On Fri, Jun 19, 2020 at 8:37 AM yangerkun <yangerkun@huawei.com> wrote:
>
> Since we support the listbadblocks command, introduce the detail in the
> doc.
>
> Signed-off-by: yangerkun <yangerkun@huawei.com>
> ---
>  .../admin-guide/device-mapper/dm-dust.rst        | 16 ++++++++++++++++
>  1 file changed, 16 insertions(+)
>
> diff --git a/Documentation/admin-guide/device-mapper/dm-dust.rst b/Documentation/admin-guide/device-mapper/dm-dust.rst
> index 84149c08b68f..5c66a71e3442 100644
> --- a/Documentation/admin-guide/device-mapper/dm-dust.rst
> +++ b/Documentation/admin-guide/device-mapper/dm-dust.rst
> @@ -205,6 +205,21 @@ If there were no bad blocks to clear, the following message will
>  appear::
>
>          no badblocks found
> +Listing the bad block list
> +--------------------------
> +
> +To list all bad blocks in the bad block list(using an example device
> +with blocks 1 and 2 in the bad block list),run the following message
> +command::

There are a couple of missing spaces here, at these two locations:

"...bad block list(using..."
change to
"...bad block list (using..."

and

"...in the bad block list),run the..."
change to
"...in the bad block list), run the..."

> +
> +        $ sudo dmsetup message dust1 0 listbadblocks
> +        1
> +        2
> +
> +No bad block exists, same message will get following report::

This would be better as the following:

"If there are no bad blocks in the bad block list, the following
message will appear"

However, note that we discussed changing this to an empty line,
instead of "null", so we would need to update the documentation to
reflect that the output will be an empty line.


Thanks,

Bryan

> +
> +        $ sudo dmsetup message dust1 0 listbadblocks
> +        null
>
>  Message commands list
>  ---------------------
> @@ -224,6 +239,7 @@ Single argument message commands::
>
>          countbadblocks
>          clearbadblocks
> +        listbadblocks
>          disable
>          enable
>          quiet
> --
> 2.25.4
>

Re: [PATCH v3 1/4] dm dust: report some message results back to user directly

[Bryan Gurney]

On Fri, Jun 19, 2020 at 1:43 PM Bryan Gurney <bgurney@redhat.com> wrote:
>
> On Fri, Jun 19, 2020 at 1:29 PM Mike Snitzer <snitzer@redhat.com> wrote:
> >
> > On Fri, Jun 19 2020 at  1:23pm -0400,
> > Bryan Gurney <bgurney@redhat.com> wrote:
> >
> > > On Fri, Jun 19, 2020 at 11:45 AM Mike Snitzer <snitzer@redhat.com> wrote:
> > > >
> > > > On Fri, Jun 19 2020 at 11:40am -0400,
> > > > Bryan Gurney <bgurney@redhat.com> wrote:
> > > >
> > > > > On Fri, Jun 19, 2020 at 8:37 AM yangerkun <yangerkun@huawei.com> wrote:
> > > > > >
> > > > > > Some type of message(queryblock/countbadblocks/removebadblock) may better
> > > > > > report results to user directly. Do it with DMEMIT.
> > > > > >
> > > > > > Signed-off-by: yangerkun <yangerkun@huawei.com>
> > > > > > ---
> > > > > >  drivers/md/dm-dust.c | 31 ++++++++++++++++++-------------
> > > > > >  1 file changed, 18 insertions(+), 13 deletions(-)
> > > > > >
> > > > > > diff --git a/drivers/md/dm-dust.c b/drivers/md/dm-dust.c
> > > > > > index ff03b90072c5..a0c75c104de0 100644
> > > > > > --- a/drivers/md/dm-dust.c
> > > > > > +++ b/drivers/md/dm-dust.c
> > > > > > @@ -138,20 +138,22 @@ static int dust_add_block(struct dust_device *dd, unsigned long long block,
> > > > > >         return 0;
> > > > > >  }
> > > > > >
> > > > > > -static int dust_query_block(struct dust_device *dd, unsigned long long block)
> > > > > > +static int dust_query_block(struct dust_device *dd, unsigned long long block, char *result,
> > > > > > +                           unsigned int maxlen, unsigned int *sz_ptr)
> > > > > >  {
> > > > > >         struct badblock *bblock;
> > > > > >         unsigned long flags;
> > > > > > +       unsigned int sz = *sz_ptr;
> > > > > >
> > > > > >         spin_lock_irqsave(&dd->dust_lock, flags);
> > > > > >         bblock = dust_rb_search(&dd->badblocklist, block);
> > > > > >         if (bblock != NULL)
> > > > > > -               DMINFO("%s: block %llu found in badblocklist", __func__, block);
> > > > > > +               DMEMIT("block %llu found in badblocklist", block);
> > > > > >         else
> > > > > > -               DMINFO("%s: block %llu not found in badblocklist", __func__, block);
> > > > > > +               DMEMIT("block %llu not found in badblocklist", block);
> > > > > >         spin_unlock_irqrestore(&dd->dust_lock, flags);
> > > > > >
> > > > > > -       return 0;
> > > > > > +       return 1;
> > > > >
> > > > > First, thank you very much for this patch.  After the concerns to
> > > > > convert some functions to use DMEMIT were brought up, I was trying to
> > > > > start the conversion, when this patch arrived, so I installed it, and
> > > > > tested it.
> > > > >
> > > > > I do have a question, though:
> > > > >
> > > > > First, I see that in dust_query_block() (above) and
> > > > > dust_clear_badblocks(), the "return 0" statement is changed to "return
> > > > > 1".
> > > > >
> > > > > (Additionally, there is a change from "r = 0" to  "r = 1", in the
> > > > > "countbadblocks" message handler)
> > > > >
> > > > > On testing the functions, they still work, but why was this change
> > > > > made?  Is it related to the use of DMEMIT?
> > > >
> > > > It is, but we need to review the returns closer.  Looked to me that 1
> > > > was being returned even if nothing was DMEMIT()'d.. but I could be wrong
> > > > (only looked quickly).
> > > >
> > > > I also noticed that some output was changed to not include __func__.
> > > > Please review that the output reflects what you'd like displayed.
> > > >
> > > > Mike
> > > >
> > >
> > > After adding __func__ back into the string, here's what it looks like:
> > >
> > > $ sudo dmsetup message dust1 0 addbadblock 60
> > > $ sudo dmsetup message dust1 0 queryblock 60
> > > dust_query_block: block 60 found in badblocklist
> > > $ sudo dmsetup message dust1 0 queryblock 61
> > > dust_query_block: block 61 not found in badblocklist
> > >
> > > I feel that the output's origin is more clear, so I'd like to leave
> > > __func__ in the output.
> > >
> > > I took a look at the DMEMIT calls, and in all three cases, it looks
> > > like there's something DMEMIT()'d:
> > >
> > > dust_query_block: either "block found", or "block not found".
> > > dust_clear_badblocks: either "no badblocks found", or "badblocks cleared".
> > > result of "message ... countbadblocks": always prints "%llu badblocks found".
> >
> > OK, can you provide an incremental patch that restores the __func__
> > where you'd like?  I can deal with it, but I'll be slower to do so.
> >
> > Mike
> >
>
> Yes, I just sent the patch to dm-devel.
>
>
> Thanks,
>
> Bryan

I'm going to work on folding the review changes above into a v4 series.


Thanks,

Bryan