This series contains a small number of bug fixes and improvements for
xen-block indirect descriptors.
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é <[email protected]>
Reported-by: Tom O'Neill <[email protected]>
Reported-by: Konrad Rzeszutek Wilk <[email protected]>
Cc: Konrad Rzeszutek Wilk <[email protected]>
---
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)
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é <[email protected]>
Reported-by: Felipe Franciosi <[email protected]>
Cc: Konrad Rzeszutek Wilk <[email protected]>
---
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)
When using certain storage devices (like RAID) having a bigger number
of segments per request provides better performance.
Signed-off-by: Roger Pau Monné <[email protected]>
Reported-by: Steven Haigh <[email protected]>
Cc: Konrad Rzeszutek Wilk <[email protected]>
---
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)
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é <[email protected]>
Cc: Konrad Rzeszutek Wilk <[email protected]>
---
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)
>>> On 21.06.13 at 12:56, Roger Pau Monne <[email protected]> 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;
>
>>> On 21.06.13 at 12:56, Roger Pau Monne <[email protected]> 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é <[email protected]>
> Reported-by: Steven Haigh <[email protected]>
> Cc: Konrad Rzeszutek Wilk <[email protected]>
> ---
> 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
> [email protected]
> http://lists.xen.org/xen-devel
On 21/06/13 13:46, Jan Beulich wrote:
>>>> On 21.06.13 at 12:56, Roger Pau Monne <[email protected]> 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;
>>
>
>
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é <[email protected]>
Cc: Konrad Rzeszutek Wilk <[email protected]>
---
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)
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? <[email protected]>
> Cc: Konrad Rzeszutek Wilk <[email protected]>
> ---
> 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)
>
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.