[PATCH v2] xen-blkfront: allow discard-* nodes to be optional

Subsystems: block layer, the rest, xen block subsystem, xen hypervisor interface

STALE2026d LANDED

Landed in mainline as 0549cd67b010 on 2021-01-26.

6 messages, 4 authors, 2021-01-27 · open the first message on its own page

[PATCH v2] xen-blkfront: allow discard-* nodes to be optional

From: Roger Pau Monne <hidden>
Date: 2021-01-19 11:18:08

This is inline with the specification described in blkif.h:

 * discard-granularity: should be set to the physical block size if
   node is not present.
 * discard-alignment, discard-secure: should be set to 0 if node not
   present.

This was detected as QEMU would only create the discard-granularity
node but not discard-alignment, and thus the setup done in
blkfront_setup_discard would fail.

Fix blkfront_setup_discard to not fail on missing nodes, and also fix
blkif_set_queue_limits to set the discard granularity to the physical
block size if none is specified in xenbus.

Fixes: ed30bf317c5ce ('xen-blkfront: Handle discard requests.')
Reported-by: Arthur Borsboom <redacted>
Signed-off-by: Roger Pau Monné <redacted>
---
Cc: Boris Ostrovsky <boris.ostrovsky@oracle.com>
Cc: Juergen Gross <jgross@suse.com>
Cc: Stefano Stabellini <sstabellini@kernel.org>
Cc: Konrad Rzeszutek Wilk <redacted>
Cc: "Roger Pau Monné" <redacted>
Cc: Jens Axboe <axboe@kernel.dk>
Cc: xen-devel@lists.xenproject.org
Cc: linux-block@vger.kernel.org
Cc: Arthur Borsboom <redacted>
---
Changes since v2:
 - Allow all discard-* nodes to be optional.
---
 drivers/block/xen-blkfront.c | 20 +++++++-------------
 1 file changed, 7 insertions(+), 13 deletions(-)

diff --git a/drivers/block/xen-blkfront.c b/drivers/block/xen-blkfront.c
index 5265975b3fba..e1c6798889f4 100644
--- a/drivers/block/xen-blkfront.c
+++ b/drivers/block/xen-blkfront.c

@@ -945,7 +945,8 @@ static void blkif_set_queue_limits(struct blkfront_info *info)
 	if (info->feature_discard) {
 		blk_queue_flag_set(QUEUE_FLAG_DISCARD, rq);
 		blk_queue_max_discard_sectors(rq, get_capacity(gd));
-		rq->limits.discard_granularity = info->discard_granularity;
+		rq->limits.discard_granularity = info->discard_granularity ?:
+						 info->physical_sector_size;
 		rq->limits.discard_alignment = info->discard_alignment;
 		if (info->feature_secdiscard)
 			blk_queue_flag_set(QUEUE_FLAG_SECERASE, rq);

@@ -2179,19 +2180,12 @@ static void blkfront_closing(struct blkfront_info *info)
 
 static void blkfront_setup_discard(struct blkfront_info *info)
 {
-	int err;
-	unsigned int discard_granularity;
-	unsigned int discard_alignment;
-
 	info->feature_discard = 1;
-	err = xenbus_gather(XBT_NIL, info->xbdev->otherend,
-		"discard-granularity", "%u", &discard_granularity,
-		"discard-alignment", "%u", &discard_alignment,
-		NULL);
-	if (!err) {
-		info->discard_granularity = discard_granularity;
-		info->discard_alignment = discard_alignment;
-	}
+	info->discard_granularity = xenbus_read_unsigned(info->xbdev->otherend,
+							 "discard-granularity",
+							 0);
+	info->discard_alignment = xenbus_read_unsigned(info->xbdev->otherend,
+						       "discard-alignment", 0);
 	info->feature_secdiscard =
 		!!xenbus_read_unsigned(info->xbdev->otherend, "discard-secure",
 				       0);

-- 
2.29.2

Re: [PATCH v2] xen-blkfront: allow discard-* nodes to be optional

From: Jürgen Groß <jgross@suse.com>
Date: 2021-01-19 11:28:56

On 19.01.21 11:57, Roger Pau Monne wrote:

This is inline with the specification described in blkif.h:

  * discard-granularity: should be set to the physical block size if
    node is not present.
  * discard-alignment, discard-secure: should be set to 0 if node not
    present.

This was detected as QEMU would only create the discard-granularity
node but not discard-alignment, and thus the setup done in
blkfront_setup_discard would fail.

Fix blkfront_setup_discard to not fail on missing nodes, and also fix
blkif_set_queue_limits to set the discard granularity to the physical
block size if none is specified in xenbus.

Fixes: ed30bf317c5ce ('xen-blkfront: Handle discard requests.')
Reported-by: Arthur Borsboom <redacted>
Signed-off-by: Roger Pau Monné <redacted>

Reviewed-by: Juergen Gross <jgross@suse.com>


Juergen

Re: [PATCH v2] xen-blkfront: allow discard-* nodes to be optional

From: Roger Pau Monné <hidden>
Date: 2021-01-19 13:03:01

Forgot to Cc stable for the Fixes tag. Doing it now.

On Tue, Jan 19, 2021 at 11:57:27AM +0100, Roger Pau Monne wrote:

quoted hunk

This is inline with the specification described in blkif.h:

 * discard-granularity: should be set to the physical block size if
   node is not present.
 * discard-alignment, discard-secure: should be set to 0 if node not
   present.

This was detected as QEMU would only create the discard-granularity
node but not discard-alignment, and thus the setup done in
blkfront_setup_discard would fail.

Fix blkfront_setup_discard to not fail on missing nodes, and also fix
blkif_set_queue_limits to set the discard granularity to the physical
block size if none is specified in xenbus.

Fixes: ed30bf317c5ce ('xen-blkfront: Handle discard requests.')
Reported-by: Arthur Borsboom <redacted>
Signed-off-by: Roger Pau Monné <redacted>
---
Cc: Boris Ostrovsky <boris.ostrovsky@oracle.com>
Cc: Juergen Gross <jgross@suse.com>
Cc: Stefano Stabellini <sstabellini@kernel.org>
Cc: Konrad Rzeszutek Wilk <redacted>
Cc: "Roger Pau Monné" <redacted>
Cc: Jens Axboe <axboe@kernel.dk>
Cc: xen-devel@lists.xenproject.org
Cc: linux-block@vger.kernel.org
Cc: Arthur Borsboom <redacted>
---
Changes since v2:
 - Allow all discard-* nodes to be optional.
---
 drivers/block/xen-blkfront.c | 20 +++++++-------------
 1 file changed, 7 insertions(+), 13 deletions(-)

diff --git a/drivers/block/xen-blkfront.c b/drivers/block/xen-blkfront.c
index 5265975b3fba..e1c6798889f4 100644
--- a/drivers/block/xen-blkfront.c
+++ b/drivers/block/xen-blkfront.c

@@ -945,7 +945,8 @@ static void blkif_set_queue_limits(struct blkfront_info *info)
 	if (info->feature_discard) {
 		blk_queue_flag_set(QUEUE_FLAG_DISCARD, rq);
 		blk_queue_max_discard_sectors(rq, get_capacity(gd));
-		rq->limits.discard_granularity = info->discard_granularity;
+		rq->limits.discard_granularity = info->discard_granularity ?:
+						 info->physical_sector_size;
 		rq->limits.discard_alignment = info->discard_alignment;
 		if (info->feature_secdiscard)
 			blk_queue_flag_set(QUEUE_FLAG_SECERASE, rq);

@@ -2179,19 +2180,12 @@ static void blkfront_closing(struct blkfront_info *info)
 
 static void blkfront_setup_discard(struct blkfront_info *info)
 {
-	int err;
-	unsigned int discard_granularity;
-	unsigned int discard_alignment;
-
 	info->feature_discard = 1;
-	err = xenbus_gather(XBT_NIL, info->xbdev->otherend,
-		"discard-granularity", "%u", &discard_granularity,
-		"discard-alignment", "%u", &discard_alignment,
-		NULL);
-	if (!err) {
-		info->discard_granularity = discard_granularity;
-		info->discard_alignment = discard_alignment;
-	}
+	info->discard_granularity = xenbus_read_unsigned(info->xbdev->otherend,
+							 "discard-granularity",
+							 0);
+	info->discard_alignment = xenbus_read_unsigned(info->xbdev->otherend,
+						       "discard-alignment", 0);
 	info->feature_secdiscard =
 		!!xenbus_read_unsigned(info->xbdev->otherend, "discard-secure",
 				       0);

-- 
2.29.2

Re: [PATCH v2] xen-blkfront: allow discard-* nodes to be optional

From: Greg KH <gregkh@linuxfoundation.org>
Date: 2021-01-19 15:25:17

On Tue, Jan 19, 2021 at 01:36:22PM +0100, Roger Pau Monné wrote:

Forgot to Cc stable for the Fixes tag. Doing it now.

<formletter>

This is not the correct way to submit patches for inclusion in the
stable kernel tree.  Please read:
    https://www.kernel.org/doc/html/latest/process/stable-kernel-rules.html
for how to do this properly.

</formletter>

Re: [PATCH v2] xen-blkfront: allow discard-* nodes to be optional

From: Arthur Borsboom <hidden>
Date: 2021-01-20 14:54:46

Hi Roger,

I have set up a test environment based on Linux 5.11.0-rc4.
The patch did not apply clean, so I copied/pasted the patch manually.

Without the patch the call trace (as reported) is visible in dmesg.
With the patch the call trace in dmesg is gone, but ... (there is
always a but) ...

Now the discard action returns the following.

[arthur@test-arch ~]$ sudo fstrim -v /
fstrim: /: the discard operation is not supported

It might be correct, but of course I was hoping the Xen VM guest would
pass on the discard request to the block device in the Xen VM host,
which is a disk partition.
Any suggestions?

(Resend due to the previous email being HTML instead of plain text).


On Tue, 19 Jan 2021 at 11:57, Roger Pau Monne [off-list ref] wrote:

quoted hunk

This is inline with the specification described in blkif.h:

 * discard-granularity: should be set to the physical block size if
   node is not present.
 * discard-alignment, discard-secure: should be set to 0 if node not
   present.

This was detected as QEMU would only create the discard-granularity
node but not discard-alignment, and thus the setup done in
blkfront_setup_discard would fail.

Fix blkfront_setup_discard to not fail on missing nodes, and also fix
blkif_set_queue_limits to set the discard granularity to the physical
block size if none is specified in xenbus.

Fixes: ed30bf317c5ce ('xen-blkfront: Handle discard requests.')
Reported-by: Arthur Borsboom <redacted>
Signed-off-by: Roger Pau Monné <redacted>
---
Cc: Boris Ostrovsky <boris.ostrovsky@oracle.com>
Cc: Juergen Gross <jgross@suse.com>
Cc: Stefano Stabellini <sstabellini@kernel.org>
Cc: Konrad Rzeszutek Wilk <redacted>
Cc: "Roger Pau Monné" <redacted>
Cc: Jens Axboe <axboe@kernel.dk>
Cc: xen-devel@lists.xenproject.org
Cc: linux-block@vger.kernel.org
Cc: Arthur Borsboom <redacted>
---
Changes since v2:
 - Allow all discard-* nodes to be optional.
---
 drivers/block/xen-blkfront.c | 20 +++++++-------------
 1 file changed, 7 insertions(+), 13 deletions(-)

diff --git a/drivers/block/xen-blkfront.c b/drivers/block/xen-blkfront.c
index 5265975b3fba..e1c6798889f4 100644
--- a/drivers/block/xen-blkfront.c
+++ b/drivers/block/xen-blkfront.c

@@ -945,7 +945,8 @@ static void blkif_set_queue_limits(struct blkfront_info *info)
        if (info->feature_discard) {
                blk_queue_flag_set(QUEUE_FLAG_DISCARD, rq);
                blk_queue_max_discard_sectors(rq, get_capacity(gd));
-               rq->limits.discard_granularity = info->discard_granularity;
+               rq->limits.discard_granularity = info->discard_granularity ?:
+                                                info->physical_sector_size;
                rq->limits.discard_alignment = info->discard_alignment;
                if (info->feature_secdiscard)
                        blk_queue_flag_set(QUEUE_FLAG_SECERASE, rq);

@@ -2179,19 +2180,12 @@ static void blkfront_closing(struct blkfront_info *info)

 static void blkfront_setup_discard(struct blkfront_info *info)
 {
-       int err;
-       unsigned int discard_granularity;
-       unsigned int discard_alignment;
-
        info->feature_discard = 1;
-       err = xenbus_gather(XBT_NIL, info->xbdev->otherend,
-               "discard-granularity", "%u", &discard_granularity,
-               "discard-alignment", "%u", &discard_alignment,
-               NULL);
-       if (!err) {
-               info->discard_granularity = discard_granularity;
-               info->discard_alignment = discard_alignment;
-       }
+       info->discard_granularity = xenbus_read_unsigned(info->xbdev->otherend,
+                                                        "discard-granularity",
+                                                        0);
+       info->discard_alignment = xenbus_read_unsigned(info->xbdev->otherend,
+                                                      "discard-alignment", 0);
        info->feature_secdiscard =
                !!xenbus_read_unsigned(info->xbdev->otherend, "discard-secure",
                                       0);
--

2.29.2


-- 
Arthur Borsboom

Re: [PATCH v2] xen-blkfront: allow discard-* nodes to be optional

From: Jürgen Groß <jgross@suse.com>
Date: 2021-01-27 08:28:30

On 19.01.21 11:57, Roger Pau Monne wrote:

This is inline with the specification described in blkif.h:

  * discard-granularity: should be set to the physical block size if
    node is not present.
  * discard-alignment, discard-secure: should be set to 0 if node not
    present.

This was detected as QEMU would only create the discard-granularity
node but not discard-alignment, and thus the setup done in
blkfront_setup_discard would fail.

Fix blkfront_setup_discard to not fail on missing nodes, and also fix
blkif_set_queue_limits to set the discard granularity to the physical
block size if none is specified in xenbus.

Fixes: ed30bf317c5ce ('xen-blkfront: Handle discard requests.')
Reported-by: Arthur Borsboom <redacted>
Signed-off-by: Roger Pau Monné <redacted>

Committed to xen/tip.git for-linus-5.11


Juergen

`h`	back out one level
`j`	next message in thread
`k`	previous message in thread
`l`	drill in
`Esc`	close help / fold thread tree
`?`	toggle this help