[PATCH 0/4] xen-block: bug fixes for indirect-descriptors

[PATCH 0/4] xen-block: bug fixes for indirect-descriptors

[Roger Pau Monne]

This series contains a small number of bug fixes and improvements for 
xen-block indirect descriptors.

[PATCH 1/4] xen-blkback: workaround compiler bug in gcc 4.1

[Roger Pau Monne]

The code generat with gcc (GCC) 4.1.2 20080704 (Red Hat 4.1.2-54)
creates an unbound loop for the second foreach_grant_safe loop in
purge_persistent_gnt.

The workaround is to avoid having this second loop and instead
perform all the work inside the first loop by adding a new variable,
clean_used, that will be set when all the desired persistent grants
have been removed and we need to iterate over the remaining ones to
remove the WAS_ACTIVE flag.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
Reported-by: Tom O'Neill <toneill@vmem.com>
Reported-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
---
 drivers/block/xen-blkback/blkback.c |   24 ++++++++++++++----------
 1 files changed, 14 insertions(+), 10 deletions(-)

diff --git a/drivers/block/xen-blkback/blkback.c b/drivers/block/xen-blkback/blkback.c
index 4119bcd..d622d86 100644
--- a/drivers/block/xen-blkback/blkback.c
+++ b/drivers/block/xen-blkback/blkback.c
@@ -341,7 +341,7 @@ static void purge_persistent_gnt(struct xen_blkif *blkif)
 	struct persistent_gnt *persistent_gnt;
 	struct rb_node *n;
 	unsigned int num_clean, total;
-	bool scan_used = false;
+	bool scan_used = false, clean_used = false;
 	struct rb_root *root;
 
 	if (blkif->persistent_gnt_c < xen_blkif_max_pgrants ||
@@ -358,9 +358,8 @@ static void purge_persistent_gnt(struct xen_blkif *blkif)
 	num_clean = (xen_blkif_max_pgrants / 100) * LRU_PERCENT_CLEAN;
 	num_clean = blkif->persistent_gnt_c - xen_blkif_max_pgrants + num_clean;
 	num_clean = min(blkif->persistent_gnt_c, num_clean);
-	if (num_clean >
-	    (blkif->persistent_gnt_c -
-	    atomic_read(&blkif->persistent_gnt_in_use)))
+	if ((num_clean == 0) ||
+	    (num_clean > (blkif->persistent_gnt_c - atomic_read(&blkif->persistent_gnt_in_use))))
 		return;
 
 	/*
@@ -383,6 +382,11 @@ purge_list:
 		BUG_ON(persistent_gnt->handle ==
 			BLKBACK_INVALID_HANDLE);
 
+		if (clean_used) {
+			clear_bit(PERSISTENT_GNT_WAS_ACTIVE, persistent_gnt->flags);
+			continue;
+		}
+
 		if (test_bit(PERSISTENT_GNT_ACTIVE, persistent_gnt->flags))
 			continue;
 		if (!scan_used &&
@@ -400,18 +404,18 @@ purge_list:
 	 * grants that were used since last purge in order to cope
 	 * with the requested num
 	 */
-	if (!scan_used) {
+	if (!scan_used && !clean_used) {
 		pr_debug(DRV_PFX "Still missing %u purged frames\n", num_clean);
 		scan_used = true;
 		goto purge_list;
 	}
 finished:
-	/* Remove the "used" flag from all the persistent grants */
-	foreach_grant_safe(persistent_gnt, n, root, node) {
-		BUG_ON(persistent_gnt->handle ==
-			BLKBACK_INVALID_HANDLE);
-		clear_bit(PERSISTENT_GNT_WAS_ACTIVE, persistent_gnt->flags);
+	if (!clean_used) {
+		pr_debug(DRV_PFX "Finished scanning for grants to clean, removing used flag\n");
+		clean_used = true;
+		goto purge_list;
 	}
+
 	blkif->persistent_gnt_c -= (total - num_clean);
 	blkif->vbd.overflow_max_grants = 0;
 
-- 
1.7.7.5 (Apple Git-26)

[PATCH 2/4] xen-blkfront: set blk_queue_max_hw_sectors correctly

[Roger Pau Monne]

Now that indirect segments are enabled blk_queue_max_hw_sectors must
be set to match the maximum number of sectors we can handle in a
request.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
Reported-by: Felipe Franciosi <felipe.franciosi@citrix.com>
Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
---
 drivers/block/xen-blkfront.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/drivers/block/xen-blkfront.c b/drivers/block/xen-blkfront.c
index 1a0f67c..2e1ee34 100644
--- a/drivers/block/xen-blkfront.c
+++ b/drivers/block/xen-blkfront.c
@@ -633,7 +633,7 @@ static int xlvbd_init_blk_queue(struct gendisk *gd, u16 sector_size,
 	/* Hard sector size and max sectors impersonate the equiv. hardware. */
 	blk_queue_logical_block_size(rq, sector_size);
 	blk_queue_physical_block_size(rq, physical_sector_size);
-	blk_queue_max_hw_sectors(rq, 512);
+	blk_queue_max_hw_sectors(rq, (segments * PAGE_SIZE) / 512);
 
 	/* Each segment in a request is up to an aligned page in size. */
 	blk_queue_segment_boundary(rq, PAGE_SIZE - 1);
-- 
1.7.7.5 (Apple Git-26)

[PATCH 3/4] xen-blkback: check the number of iovecs before allocating a bios

[Roger Pau Monne]

With the introduction of indirect segments we can receive requests
with a number of segments bigger than the maximum number of allowed
iovecs in a bios, so make sure that blkback doesn't try to allocate a
bios with more iovecs than BIO_MAX_PAGES

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
---
 drivers/block/xen-blkback/blkback.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/drivers/block/xen-blkback/blkback.c b/drivers/block/xen-blkback/blkback.c
index d622d86..876116b 100644
--- a/drivers/block/xen-blkback/blkback.c
+++ b/drivers/block/xen-blkback/blkback.c
@@ -1236,7 +1236,8 @@ static int dispatch_rw_block_io(struct xen_blkif *blkif,
 				     seg[i].nsec << 9,
 				     seg[i].offset) == 0)) {
 
-			bio = bio_alloc(GFP_KERNEL, nseg-i);
+			int nr_iovecs = (nseg-i) > BIO_MAX_PAGES ? BIO_MAX_PAGES : (nseg-i);
+			bio = bio_alloc(GFP_KERNEL, nr_iovecs);
 			if (unlikely(bio == NULL))
 				goto fail_put_bio;
 
-- 
1.7.7.5 (Apple Git-26)

Re: [Xen-devel] [PATCH 3/4] xen-blkback: check the number of iovecs before allocating a bios

[Jan Beulich]

>>> On 21.06.13 at 12:56, Roger Pau Monne <roger.pau@citrix.com> wrote:
> @@ -1236,7 +1236,8 @@ static int dispatch_rw_block_io(struct xen_blkif *blkif,
>  				     seg[i].nsec << 9,
>  				     seg[i].offset) == 0)) {
>  
> -			bio = bio_alloc(GFP_KERNEL, nseg-i);
> +			int nr_iovecs = (nseg-i) > BIO_MAX_PAGES ? BIO_MAX_PAGES : (nseg-i);

I'm sure this results in a compiler warning (declaration after
statement).

And it surely would read much better if you used some form of
min() - the shorter line would at once allow you to properly
blank separate operands from operators.

Jan

> +			bio = bio_alloc(GFP_KERNEL, nr_iovecs);
>  			if (unlikely(bio == NULL))
>  				goto fail_put_bio;
>  

Re: [Xen-devel] [PATCH 3/4] xen-blkback: check the number of iovecs before allocating a bios

[Roger Pau Monné]

On 21/06/13 13:46, Jan Beulich wrote:
>>>> On 21.06.13 at 12:56, Roger Pau Monne <roger.pau@citrix.com> wrote:
>> @@ -1236,7 +1236,8 @@ static int dispatch_rw_block_io(struct xen_blkif *blkif,
>>  				     seg[i].nsec << 9,
>>  				     seg[i].offset) == 0)) {
>>  
>> -			bio = bio_alloc(GFP_KERNEL, nseg-i);
>> +			int nr_iovecs = (nseg-i) > BIO_MAX_PAGES ? BIO_MAX_PAGES : (nseg-i);
> 
> I'm sure this results in a compiler warning (declaration after
> statement).

No, because it's the first statement inside the while loop.

> And it surely would read much better if you used some form of
> min() - the shorter line would at once allow you to properly
> blank separate operands from operators.

Sure, I will switch it to min, thanks for the comments.

> 
> Jan
> 
>> +			bio = bio_alloc(GFP_KERNEL, nr_iovecs);
>>  			if (unlikely(bio == NULL))
>>  				goto fail_put_bio;
>>  
> 
> 

[PATCH v2 3/4] xen-blkback: check the number of iovecs before allocating a bios

[Roger Pau Monne]

With the introduction of indirect segments we can receive requests
with a number of segments bigger than the maximum number of allowed
iovecs in a bios, so make sure that blkback doesn't try to allocate a
bios with more iovecs than BIO_MAX_PAGES

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
---
 drivers/block/xen-blkback/blkback.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/drivers/block/xen-blkback/blkback.c b/drivers/block/xen-blkback/blkback.c
index d622d86..b3897f5d 100644
--- a/drivers/block/xen-blkback/blkback.c
+++ b/drivers/block/xen-blkback/blkback.c
@@ -1236,7 +1236,8 @@ static int dispatch_rw_block_io(struct xen_blkif *blkif,
 				     seg[i].nsec << 9,
 				     seg[i].offset) == 0)) {
 
-			bio = bio_alloc(GFP_KERNEL, nseg-i);
+			int nr_iovecs = min_t(int, (nseg-i), BIO_MAX_PAGES);
+			bio = bio_alloc(GFP_KERNEL, nr_iovecs);
 			if (unlikely(bio == NULL))
 				goto fail_put_bio;
 
-- 
1.7.7.5 (Apple Git-26)

Re: [PATCH v2 3/4] xen-blkback: check the number of iovecs before allocating a bios

[Konrad Rzeszutek Wilk]

On Sat, Jun 22, 2013 at 09:59:17AM +0200, Roger Pau Monne wrote:
> With the introduction of indirect segments we can receive requests
> with a number of segments bigger than the maximum number of allowed
> iovecs in a bios, so make sure that blkback doesn't try to allocate a
> bios with more iovecs than BIO_MAX_PAGES

Shouldn't we just gate the feature-indirect-descriptor value to
take this into account?

What happens if the nseg is > BIO_MAX_PAGES? Do we "lose" the request
for the remaining segments?

> 
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> ---
>  drivers/block/xen-blkback/blkback.c |    3 ++-
>  1 files changed, 2 insertions(+), 1 deletions(-)
> 
> diff --git a/drivers/block/xen-blkback/blkback.c b/drivers/block/xen-blkback/blkback.c
> index d622d86..b3897f5d 100644
> --- a/drivers/block/xen-blkback/blkback.c
> +++ b/drivers/block/xen-blkback/blkback.c
> @@ -1236,7 +1236,8 @@ static int dispatch_rw_block_io(struct xen_blkif *blkif,
>  				     seg[i].nsec << 9,
>  				     seg[i].offset) == 0)) {
>  
> -			bio = bio_alloc(GFP_KERNEL, nseg-i);
> +			int nr_iovecs = min_t(int, (nseg-i), BIO_MAX_PAGES);
> +			bio = bio_alloc(GFP_KERNEL, nr_iovecs);
>  			if (unlikely(bio == NULL))
>  				goto fail_put_bio;
>  
> -- 
> 1.7.7.5 (Apple Git-26)
> 

Re: [PATCH v2 3/4] xen-blkback: check the number of iovecs before allocating a bios

[Roger Pau Monné]

On 24/06/13 15:28, Konrad Rzeszutek Wilk wrote:
> On Sat, Jun 22, 2013 at 09:59:17AM +0200, Roger Pau Monne wrote:
>> With the introduction of indirect segments we can receive requests
>> with a number of segments bigger than the maximum number of allowed
>> iovecs in a bios, so make sure that blkback doesn't try to allocate a
>> bios with more iovecs than BIO_MAX_PAGES
> 
> Shouldn't we just gate the feature-indirect-descriptor value to
> take this into account?
> 
> What happens if the nseg is > BIO_MAX_PAGES? Do we "lose" the request
> for the remaining segments?

No, we just allocate several bios in order to send the request to the
underlying device.

[PATCH 4/4] xen-blkfront: increase the default number of indirect segments

[Roger Pau Monne]

When using certain storage devices (like RAID) having a bigger number
of segments per request provides better performance.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
Reported-by: Steven Haigh <netwiz@crc.id.au>
Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
---
 drivers/block/xen-blkfront.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/block/xen-blkfront.c b/drivers/block/xen-blkfront.c
index 2e1ee34..4e3ab34 100644
--- a/drivers/block/xen-blkfront.c
+++ b/drivers/block/xen-blkfront.c
@@ -94,9 +94,9 @@ static const struct block_device_operations xlvbd_block_fops;
  * by the backend driver.
  */
 
-static unsigned int xen_blkif_max_segments = 32;
+static unsigned int xen_blkif_max_segments = 64;
 module_param_named(max, xen_blkif_max_segments, int, S_IRUGO);
-MODULE_PARM_DESC(max, "Maximum amount of segments in indirect requests (default is 32)");
+MODULE_PARM_DESC(max, "Maximum amount of segments in indirect requests (default is 64)");
 
 #define BLK_RING_SIZE __CONST_RING_SIZE(blkif, PAGE_SIZE)
 
-- 
1.7.7.5 (Apple Git-26)

Re: [Xen-devel] [PATCH 4/4] xen-blkfront: increase the default number of indirect segments

[Jan Beulich]

>>> On 21.06.13 at 12:56, Roger Pau Monne <roger.pau@citrix.com> wrote:
> When using certain storage devices (like RAID) having a bigger number
> of segments per request provides better performance.

And there's no drawback (higher memory foot print if nothing else)
on "certain other storage devices"? Adjusting defaults just because
it is beneficial for some devices, but may adversely affect others
is not really a good thing - in such an event, you'd be better off
determining the default dynamically per device.

Jan

> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> Reported-by: Steven Haigh <netwiz@crc.id.au>
> Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> ---
>  drivers/block/xen-blkfront.c |    4 ++--
>  1 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/block/xen-blkfront.c b/drivers/block/xen-blkfront.c
> index 2e1ee34..4e3ab34 100644
> --- a/drivers/block/xen-blkfront.c
> +++ b/drivers/block/xen-blkfront.c
> @@ -94,9 +94,9 @@ static const struct block_device_operations xlvbd_block_fops;
>   * by the backend driver.
>   */
>  
> -static unsigned int xen_blkif_max_segments = 32;
> +static unsigned int xen_blkif_max_segments = 64;
>  module_param_named(max, xen_blkif_max_segments, int, S_IRUGO);
> -MODULE_PARM_DESC(max, "Maximum amount of segments in indirect requests 
> (default is 32)");
> +MODULE_PARM_DESC(max, "Maximum amount of segments in indirect requests 
> (default is 64)");
>  
>  #define BLK_RING_SIZE __CONST_RING_SIZE(blkif, PAGE_SIZE)
>  
> -- 
> 1.7.7.5 (Apple Git-26)
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org 
> http://lists.xen.org/xen-devel