Segments can span multiple clusters with tap:qcow

PATCH In review Mark McLoughlin 3 participants 9 messages Apr 25, 2007 – May 3, 2007

Share & tools

Mark McLoughlin

author

2007-04-25 20:41 UTC #0 lore raw

PATCH 1/1 Segments can span multiple clusters with tap:qcow

Hey,
In blktap's qcow we need split up read/write requests if the requests
span multiple clusters. However, with our MAX_AIO_REQUESTS define we assume that there is only ever a single aio request per tapdisk request and under heavy i/o we can run out of room causing us to cancel requests.

The attached patch dynamically allocates (based on cluster_bits) the
various io request queues the driver maintains.

Signed-off-by Mark McLoughlin <[email protected]>

Cheers,
Mark.

tools/blktap/drivers/block-qcow.c +53 ~~-14~~

diff -r 33e22185002a tools/blktap/drivers/block-qcow.c
@@ -55,7 +55,6 @@
 
 /******AIO DEFINES******/
 #define REQUEST_ASYNC_FD 1
-#define MAX_AIO_REQS (MAX_REQUESTS * MAX_SEGMENTS_PER_REQ)
 
 struct pending_aio {
         td_callback_t cb;

@@ -146,18 +145,37 @@ struct tdqcow_state {
 	AES_KEY aes_encrypt_key;       /*AES key*/
 	AES_KEY aes_decrypt_key;       /*AES key*/
         /* libaio state */
-        io_context_t       aio_ctx;
-        struct iocb        iocb_list  [MAX_AIO_REQS];
-        struct iocb       *iocb_free  [MAX_AIO_REQS];
-        struct pending_aio pending_aio[MAX_AIO_REQS];
-        int                iocb_free_count;
-        struct iocb       *iocb_queue[MAX_AIO_REQS];
-        int                iocb_queued;
-        int                poll_fd;      /* NB: we require aio_poll support */
-        struct io_event    aio_events[MAX_AIO_REQS];
+        io_context_t        aio_ctx;
+        int                 max_aio_reqs;
+        struct iocb        *iocb_list;
+        struct iocb       **iocb_free;
+        struct pending_aio *pending_aio;
+        int                 iocb_free_count;
+        struct iocb       **iocb_queue;
+        int                 iocb_queued;
+        int                 poll_fd;      /* NB: we require aio_poll support */
+        struct io_event    *aio_events;
 };
 
 static int decompress_cluster(struct tdqcow_state *s, uint64_t cluster_offset);
+
+static void free_aio_state(struct td_state *bs)
+{
+	struct tdqcow_state *s = (struct tdqcow_state *)bs->private;
+
+        if (s->sector_lock)
+                free(s->sector_lock);
+        if (s->iocb_list)
+                free(s->iocb_list);
+        if (s->pending_aio)
+                free(s->pending_aio);
+        if (s->aio_events)
+                free(s->aio_events);
+        if (s->iocb_free)
+                free(s->iocb_free);
+        if (s->iocb_queue)
+                free(s->iocb_queue);
+}
 
 static int init_aio_state(struct disk_driver *dd)
 {

@@ -166,6 +184,12 @@ static int init_aio_state(struct disk_dr
 	struct tdqcow_state  *s = (struct tdqcow_state *)dd->private;
         long     ioidx;
 
+        s->iocb_list = NULL;
+        s->pending_aio = NULL;
+        s->aio_events = NULL;
+        s->iocb_free = NULL;
+        s->iocb_queue = NULL;
+
         /*Initialize Locking bitmap*/
 	s->sector_lock = calloc(1, bs->size);

@@ -174,13 +198,26 @@ static int init_aio_state(struct disk_dr
 		goto fail;
 	}
 
+        /* A segment (i.e. a page) can span multiple clusters */
+        s->max_aio_reqs = (getpagesize() / s->cluster_size) + 1;
+
         /* Initialize AIO */
-        s->iocb_free_count = MAX_AIO_REQS;
+        s->iocb_free_count = s->max_aio_reqs;
         s->iocb_queued     = 0;
+
+        if (!(s->iocb_list = malloc(sizeof(struct iocb) * s->max_aio_reqs)) ||
+            !(s->pending_aio = malloc(sizeof(struct pending_aio) * s->max_aio_reqs)) ||
+            !(s->aio_events = malloc(sizeof(struct io_event) * s->max_aio_reqs)) ||
+            !(s->iocb_free = malloc(sizeof(struct iocb *) * s->max_aio_reqs)) ||
+            !(s->iocb_queue = malloc(sizeof(struct iocb *) * s->max_aio_reqs))) {
+                DPRINTF("Failed to allocate AIO structs (max_aio_reqs = %d)\n",
+                        s->max_aio_reqs);
+                goto fail;
+        }
 
         /*Signal kernel to create Poll FD for Asyc completion events*/
         s->aio_ctx = (io_context_t) REQUEST_ASYNC_FD;   
-        s->poll_fd = io_setup(MAX_AIO_REQS, &s->aio_ctx);
+        s->poll_fd = io_setup(s->max_aio_reqs, &s->aio_ctx);
 
 	if (s->poll_fd < 0) {
                 if (s->poll_fd == -EAGAIN) {

@@ -198,7 +235,7 @@ static int init_aio_state(struct disk_dr
 		goto fail;
 	}
 
-        for (i=0;i<MAX_AIO_REQS;i++)
+        for (i=0;i<s->max_aio_reqs;i++)
                 s->iocb_free[i] = &s->iocb_list[i];
 
         DPRINTF("AIO state initialised\n");

@@ -946,6 +983,7 @@ int tdqcow_open (struct disk_driver *dd,
  end_xenhdr:
 	if (init_aio_state(dd)!=0) {
 		DPRINTF("Unable to initialise AIO state\n");
+                free_aio_state(bs);
 		goto fail;
 	}
 	init_fds(dd);

@@ -962,6 +1000,7 @@ int tdqcow_open (struct disk_driver *dd,
 	
 fail:
 	DPRINTF("QCOW Open failed\n");
+	free_aio_state(bs);
 	free(s->l1_table);
 	free(s->l2_cache);
 	free(s->cluster_cache);

@@ -1145,7 +1184,7 @@ int tdqcow_do_callbacks(struct disk_driv
         if (sid > MAX_IOFD) return 1;
 	
 	/* Non-blocking test for completed io. */
-        ret = io_getevents(prv->aio_ctx, 0, MAX_AIO_REQS, prv->aio_events,
+        ret = io_getevents(prv->aio_ctx, 0, prv->max_aio_reqs, prv->aio_events,
                            NULL);
 
         for (ep = prv->aio_events, i = ret; i-- > 0; ep++) {

_______________________________________________
Xen-devel mailing list
[email protected]
http://lists.xensource.com/xen-devel

Keir Fraser

2007-04-26 09:09 UTC #1 lore raw

Quoted 8 lines · Mark McLoughlin

In blktap's qcow we need split up read/write requests if the requests span multiple clusters. However, with our MAX_AIO_REQUESTS define we assume that there is only ever a single aio request per tapdisk request and under heavy i/o we can run out of room causing us to cancel requests.
The attached patch dynamically allocates (based on cluster_bits) the various io request queues the driver maintains.

The current code allocates aio-request info for every segment in a request ring (MAX_AIO_REQUESTS == BLK_RING_SIZE * MAX_SEGMENTS_PER_REQUEST). This patch seems to take into account that each segment (part-of-page) can itself be split into clusters, hence the page_size/cluster_size calculation, but shouldn't this be multiplied by the existing MAX_AIO_REQUESTS? Otherwise you provide only enough aio requests for one segment at a time, rather than a request ring's worth of segments?

-- Keir

Mark McLoughlin

author

2007-04-26 09:18 UTC #2 lore raw

Hi Keir,

Quoted 11 lines · Keir Fraser

Quoted 8 lines · Mark McLoughlin
In blktap's qcow we need split up read/write requests if the requests span multiple clusters. However, with our MAX_AIO_REQUESTS define we assume that there is only ever a single aio request per tapdisk request and under heavy i/o we can run out of room causing us to cancel requests.
The attached patch dynamically allocates (based on cluster_bits) the various io request queues the driver maintains.

Quoted 7 lines

The current code allocates aio-request info for every segment in a request ring (MAX_AIO_REQUESTS == BLK_RING_SIZE * MAX_SEGMENTS_PER_REQUEST). This patch seems to take into account that each segment (part-of-page) can itself be split into clusters, hence the page_size/cluster_size calculation, but shouldn't this be multiplied by the existing MAX_AIO_REQUESTS? Otherwise you provide only enough aio requests for one segment at a time, rather than a request ring's worth of segments?

Absolutely, well spotted. I fixed that typo after testing, but
obviously forgot to run "quilt refresh" before sending ...

Fixed version attached.

Thanks,
Mark.

tools/blktap/drivers/block-qcow.c +53 ~~-14~~

diff -r 33e22185002a tools/blktap/drivers/block-qcow.c
@@ -55,7 +55,6 @@
 
 /******AIO DEFINES******/
 #define REQUEST_ASYNC_FD 1
-#define MAX_AIO_REQS (MAX_REQUESTS * MAX_SEGMENTS_PER_REQ)
 
 struct pending_aio {
         td_callback_t cb;

@@ -146,18 +145,37 @@ struct tdqcow_state {
 	AES_KEY aes_encrypt_key;       /*AES key*/
 	AES_KEY aes_decrypt_key;       /*AES key*/
         /* libaio state */
-        io_context_t       aio_ctx;
-        struct iocb        iocb_list  [MAX_AIO_REQS];
-        struct iocb       *iocb_free  [MAX_AIO_REQS];
-        struct pending_aio pending_aio[MAX_AIO_REQS];
-        int                iocb_free_count;
-        struct iocb       *iocb_queue[MAX_AIO_REQS];
-        int                iocb_queued;
-        int                poll_fd;      /* NB: we require aio_poll support */
-        struct io_event    aio_events[MAX_AIO_REQS];
+        io_context_t        aio_ctx;
+        int                 max_aio_reqs;
+        struct iocb        *iocb_list;
+        struct iocb       **iocb_free;
+        struct pending_aio *pending_aio;
+        int                 iocb_free_count;
+        struct iocb       **iocb_queue;
+        int                 iocb_queued;
+        int                 poll_fd;      /* NB: we require aio_poll support */
+        struct io_event    *aio_events;
 };
 
 static int decompress_cluster(struct tdqcow_state *s, uint64_t cluster_offset);
+
+static void free_aio_state(struct td_state *bs)
+{
+	struct tdqcow_state *s = (struct tdqcow_state *)bs->private;
+
+        if (s->sector_lock)
+                free(s->sector_lock);
+        if (s->iocb_list)
+                free(s->iocb_list);
+        if (s->pending_aio)
+                free(s->pending_aio);
+        if (s->aio_events)
+                free(s->aio_events);
+        if (s->iocb_free)
+                free(s->iocb_free);
+        if (s->iocb_queue)
+                free(s->iocb_queue);
+}
 
 static int init_aio_state(struct disk_driver *dd)
 {

@@ -166,6 +184,12 @@ static int init_aio_state(struct disk_dr
 	struct tdqcow_state  *s = (struct tdqcow_state *)dd->private;
         long     ioidx;
 
+        s->iocb_list = NULL;
+        s->pending_aio = NULL;
+        s->aio_events = NULL;
+        s->iocb_free = NULL;
+        s->iocb_queue = NULL;
+
         /*Initialize Locking bitmap*/
 	s->sector_lock = calloc(1, bs->size);

@@ -174,13 +198,26 @@ static int init_aio_state(struct disk_dr
 		goto fail;
 	}
 
+        /* A segment (i.e. a page) can span multiple clusters */
+        s->max_aio_reqs = ((getpagesize() / s->cluster_size) + 1) * MAX_SEGMENTS_PER_REQ * MAX_REQUESTS;
+
         /* Initialize AIO */
-        s->iocb_free_count = MAX_AIO_REQS;
+        s->iocb_free_count = s->max_aio_reqs;
         s->iocb_queued     = 0;
+
+        if (!(s->iocb_list = malloc(sizeof(struct iocb) * s->max_aio_reqs)) ||
+            !(s->pending_aio = malloc(sizeof(struct pending_aio) * s->max_aio_reqs)) ||
+            !(s->aio_events = malloc(sizeof(struct io_event) * s->max_aio_reqs)) ||
+            !(s->iocb_free = malloc(sizeof(struct iocb *) * s->max_aio_reqs)) ||
+            !(s->iocb_queue = malloc(sizeof(struct iocb *) * s->max_aio_reqs))) {
+                DPRINTF("Failed to allocate AIO structs (max_aio_reqs = %d)\n",
+                        s->max_aio_reqs);
+                goto fail;
+        }
 
         /*Signal kernel to create Poll FD for Asyc completion events*/
         s->aio_ctx = (io_context_t) REQUEST_ASYNC_FD;   
-        s->poll_fd = io_setup(MAX_AIO_REQS, &s->aio_ctx);
+        s->poll_fd = io_setup(s->max_aio_reqs, &s->aio_ctx);
 
 	if (s->poll_fd < 0) {
                 if (s->poll_fd == -EAGAIN) {

@@ -198,7 +235,7 @@ static int init_aio_state(struct disk_dr
 		goto fail;
 	}
 
-        for (i=0;i<MAX_AIO_REQS;i++)
+        for (i=0;i<s->max_aio_reqs;i++)
                 s->iocb_free[i] = &s->iocb_list[i];
 
         DPRINTF("AIO state initialised\n");

@@ -946,6 +983,7 @@ int tdqcow_open (struct disk_driver *dd,
  end_xenhdr:
 	if (init_aio_state(dd)!=0) {
 		DPRINTF("Unable to initialise AIO state\n");
+                free_aio_state(bs);
 		goto fail;
 	}
 	init_fds(dd);

@@ -962,6 +1000,7 @@ int tdqcow_open (struct disk_driver *dd,
 	
 fail:
 	DPRINTF("QCOW Open failed\n");
+	free_aio_state(bs);
 	free(s->l1_table);
 	free(s->l2_cache);
 	free(s->cluster_cache);

@@ -1145,7 +1184,7 @@ int tdqcow_do_callbacks(struct disk_driv
         if (sid > MAX_IOFD) return 1;
 	
 	/* Non-blocking test for completed io. */
-        ret = io_getevents(prv->aio_ctx, 0, MAX_AIO_REQS, prv->aio_events,
+        ret = io_getevents(prv->aio_ctx, 0, prv->max_aio_reqs, prv->aio_events,
                            NULL);
 
         for (ep = prv->aio_events, i = ret; i-- > 0; ep++) {

_______________________________________________
Xen-devel mailing list
[email protected]
http://lists.xensource.com/xen-devel

Keir Fraser

2007-04-26 10:00 UTC #3 lore raw

Quoted 8 lines · Mark McLoughlin

Quoted 7 lines
The current code allocates aio-request info for every segment in a request ring (MAX_AIO_REQUESTS == BLK_RING_SIZE * MAX_SEGMENTS_PER_REQUEST). This patch seems to take into account that each segment (part-of-page) can itself be split into clusters, hence the page_size/cluster_size calculation, but shouldn't this be multiplied by the existing MAX_AIO_REQUESTS? Otherwise you provide only enough aio requests for one segment at a time, rather than a request ring's worth of segments?

Quoted 4 lines

Absolutely, well spotted. I fixed that typo after testing, but obviously forgot to run "quilt refresh" before sending ...
Fixed version attached.

This one doesn't build (free_aio_state: line 164: structure has no member named 'private'). Perhaps free_aio_state() should take a 'struct disk_driver' rather than a 'struct td_state'?

-- Keir

Mark McLoughlin

author

2007-04-26 10:21 UTC #4 lore raw

Quoted 10 lines · Keir Fraser

Quoted 8 lines · Mark McLoughlin
Quoted 7 lines
The current code allocates aio-request info for every segment in a request ring (MAX_AIO_REQUESTS == BLK_RING_SIZE * MAX_SEGMENTS_PER_REQUEST). This patch seems to take into account that each segment (part-of-page) can itself be split into clusters, hence the page_size/cluster_size calculation, but shouldn't this be multiplied by the existing MAX_AIO_REQUESTS? Otherwise you provide only enough aio requests for one segment at a time, rather than a request ring's worth of segments?

Quoted 8 lines

Quoted 4 lines
Absolutely, well spotted. I fixed that typo after testing, but obviously forgot to run "quilt refresh" before sending ...
Fixed version attached.
This one doesn't build (free_aio_state: line 164: structure has no member named 'private'). Perhaps free_aio_state() should take a 'struct disk_driver' rather than a 'struct td_state'?

Gah, merge error going from 3.0.4 to 3.0.5. This one builds.

Thanks,
Mark.

tools/blktap/drivers/block-qcow.c +53 ~~-14~~

diff -r 33e22185002a tools/blktap/drivers/block-qcow.c
@@ -55,7 +55,6 @@
 
 /******AIO DEFINES******/
 #define REQUEST_ASYNC_FD 1
-#define MAX_AIO_REQS (MAX_REQUESTS * MAX_SEGMENTS_PER_REQ)
 
 struct pending_aio {
         td_callback_t cb;

@@ -146,18 +145,37 @@ struct tdqcow_state {
 	AES_KEY aes_encrypt_key;       /*AES key*/
 	AES_KEY aes_decrypt_key;       /*AES key*/
         /* libaio state */
-        io_context_t       aio_ctx;
-        struct iocb        iocb_list  [MAX_AIO_REQS];
-        struct iocb       *iocb_free  [MAX_AIO_REQS];
-        struct pending_aio pending_aio[MAX_AIO_REQS];
-        int                iocb_free_count;
-        struct iocb       *iocb_queue[MAX_AIO_REQS];
-        int                iocb_queued;
-        int                poll_fd;      /* NB: we require aio_poll support */
-        struct io_event    aio_events[MAX_AIO_REQS];
+        io_context_t        aio_ctx;
+        int                 max_aio_reqs;
+        struct iocb        *iocb_list;
+        struct iocb       **iocb_free;
+        struct pending_aio *pending_aio;
+        int                 iocb_free_count;
+        struct iocb       **iocb_queue;
+        int                 iocb_queued;
+        int                 poll_fd;      /* NB: we require aio_poll support */
+        struct io_event    *aio_events;
 };
 
 static int decompress_cluster(struct tdqcow_state *s, uint64_t cluster_offset);
+
+static void free_aio_state(struct disk_driver *dd)
+{
+	struct tdqcow_state *s = (struct disk_driver *)dd->private;
+
+        if (s->sector_lock)
+                free(s->sector_lock);
+        if (s->iocb_list)
+                free(s->iocb_list);
+        if (s->pending_aio)
+                free(s->pending_aio);
+        if (s->aio_events)
+                free(s->aio_events);
+        if (s->iocb_free)
+                free(s->iocb_free);
+        if (s->iocb_queue)
+                free(s->iocb_queue);
+}
 
 static int init_aio_state(struct disk_driver *dd)
 {

@@ -166,6 +184,12 @@ static int init_aio_state(struct disk_dr
 	struct tdqcow_state  *s = (struct tdqcow_state *)dd->private;
         long     ioidx;
 
+        s->iocb_list = NULL;
+        s->pending_aio = NULL;
+        s->aio_events = NULL;
+        s->iocb_free = NULL;
+        s->iocb_queue = NULL;
+
         /*Initialize Locking bitmap*/
 	s->sector_lock = calloc(1, bs->size);

@@ -174,13 +198,26 @@ static int init_aio_state(struct disk_dr
 		goto fail;
 	}
 
+        /* A segment (i.e. a page) can span multiple clusters */
+        s->max_aio_reqs = (getpagesize() / s->cluster_size) + 1;
+
         /* Initialize AIO */
-        s->iocb_free_count = MAX_AIO_REQS;
+        s->iocb_free_count = s->max_aio_reqs;
         s->iocb_queued     = 0;
+
+        if (!(s->iocb_list = malloc(sizeof(struct iocb) * s->max_aio_reqs)) ||
+            !(s->pending_aio = malloc(sizeof(struct pending_aio) * s->max_aio_reqs)) ||
+            !(s->aio_events = malloc(sizeof(struct io_event) * s->max_aio_reqs)) ||
+            !(s->iocb_free = malloc(sizeof(struct iocb *) * s->max_aio_reqs)) ||
+            !(s->iocb_queue = malloc(sizeof(struct iocb *) * s->max_aio_reqs))) {
+                DPRINTF("Failed to allocate AIO structs (max_aio_reqs = %d)\n",
+                        s->max_aio_reqs);
+                goto fail;
+        }
 
         /*Signal kernel to create Poll FD for Asyc completion events*/
         s->aio_ctx = (io_context_t) REQUEST_ASYNC_FD;   
-        s->poll_fd = io_setup(MAX_AIO_REQS, &s->aio_ctx);
+        s->poll_fd = io_setup(s->max_aio_reqs, &s->aio_ctx);
 
 	if (s->poll_fd < 0) {
                 if (s->poll_fd == -EAGAIN) {

@@ -198,7 +235,7 @@ static int init_aio_state(struct disk_dr
 		goto fail;
 	}
 
-        for (i=0;i<MAX_AIO_REQS;i++)
+        for (i=0;i<s->max_aio_reqs;i++)
                 s->iocb_free[i] = &s->iocb_list[i];
 
         DPRINTF("AIO state initialised\n");

@@ -946,6 +983,7 @@ int tdqcow_open (struct disk_driver *dd,
  end_xenhdr:
 	if (init_aio_state(dd)!=0) {
 		DPRINTF("Unable to initialise AIO state\n");
+                free_aio_state(dd);
 		goto fail;
 	}
 	init_fds(dd);

@@ -962,6 +1000,7 @@ int tdqcow_open (struct disk_driver *dd,
 	
 fail:
 	DPRINTF("QCOW Open failed\n");
+	free_aio_state(dd);
 	free(s->l1_table);
 	free(s->l2_cache);
 	free(s->cluster_cache);

@@ -1145,7 +1184,7 @@ int tdqcow_do_callbacks(struct disk_driv
         if (sid > MAX_IOFD) return 1;
 	
 	/* Non-blocking test for completed io. */
-        ret = io_getevents(prv->aio_ctx, 0, MAX_AIO_REQS, prv->aio_events,
+        ret = io_getevents(prv->aio_ctx, 0, prv->max_aio_reqs, prv->aio_events,
                            NULL);
 
         for (ep = prv->aio_events, i = ret; i-- > 0; ep++) {

_______________________________________________
Xen-devel mailing list
[email protected]
http://lists.xensource.com/xen-devel

Jake Wires

2007-05-03 01:06 UTC #5 lore raw

RE: [PATCH] Segments can span multiple clusters withtap:qcow

Hi,

This patch is back to only allocating enough requests for one segment:

+ /* A segment (i.e. a page) can span multiple clusters */
+ s->max_aio_reqs = (getpagesize() / s->cluster_size) + 1;

In fact, this code allocates exactly two AIO requests for QCoW images created by qcow-create, which have a default cluster size of 4K.

For a while now, tapdisk has supported EBUSY -- that is, if a plugin returns -EBUSY to tapdisk, tapdisk will put the last segment back on its queue and wait until the plugin has made progress before reissuing the request. Thus users should not observe an error when QCoW runs out of AIO requests. This is attested by the fact that even with only 2 AIO requests allocated, QCoW block devices can handle a heavy load: I just mkfs'ed and copied a 1GB file to a QCoW image with no problem -- although it took quite a long while to do so, since only two segments were served at a time ;).

If you were observing errors while writing to QCoW devices, I'd like to know how you were causing them -- we may need to make some other changes to fix them. However, I'm not convinced that this patch is necessary.

Thanks,
Jake

Quoted 9 lines

-----Original Message-----
From: [email protected] [mailto:xen-devel- [email protected]] On Behalf Of Mark McLoughlin
Sent: Thursday, April 26, 2007 3:21 AM
To: Keir Fraser
Cc: xen-devel
Subject: Re: [Xen-devel] [PATCH] Segments can span multiple clusters withtap:qcow

Quoted 4 lines

Quoted 3 lines · Keir Fraser
Quoted 1 line · Mark McLoughlin
Quoted 1 line
The current code allocates aio-request info for every segment in

Quoted 2 lines

request
Quoted 1 line
Quoted 1 line
Quoted 1 line
ring (MAX_AIO_REQUESTS == BLK_RING_SIZE *

MAX_SEGMENTS_PER_REQUEST).

Quoted 2 lines

This
Quoted 1 line
Quoted 1 line
Quoted 1 line
patch seems to take into account that each segment (part-of-page)

can

Quoted 2 lines

itself
Quoted 1 line
Quoted 1 line
Quoted 1 line
be split into clusters, hence the page_size/cluster_size

calculation,

Quoted 4 lines

but
Quoted 1 line
Quoted 1 line
Quoted 1 line
shouldn't this be multiplied by the existing MAX_AIO_REQUESTS?
Otherwise you
Quoted 1 line
Quoted 1 line
Quoted 1 line
provide only enough aio requests for one segment at a time,

rather

Quoted 17 lines

than a
Quoted 8 lines
Quoted 6 lines
Quoted 1 line
request ring's worth of segments?
Absolutely, well spotted. I fixed that typo after testing, but obviously forgot to run "quilt refresh" before sending ...
Fixed version attached.
This one doesn't build (free_aio_state: line 164: structure has no
member
Quoted 2 lines
named 'private'). Perhaps free_aio_state() should take a 'struct disk_driver' rather than a 'struct td_state'?
Gah, merge error going from 3.0.4 to 3.0.5. This one builds.
Thanks,
Mark.

Keir Fraser

2007-05-03 17:40 UTC #6 lore raw

Re: [PATCH] Segments can span multiple clusters withtap:qcow

Quoted 7 lines · Jake Wires

This patch is back to only allocating enough requests for one segment:
tools/blktap/drivers/block-qcow.c +2 -0
+        /* A segment (i.e. a page) can span multiple clusters */
+        s->max_aio_reqs = (getpagesize() / s->cluster_size) + 1;
In fact, this code allocates exactly two AIO requests for QCoW images created by qcow-create, which have a default cluster size of 4K.

What shall we do -- revert the whole patch or fix this line?

-- Keir

Mark McLoughlin

author

2007-05-03 17:47 UTC #7 lore raw

Re: [PATCH] Segments can span multiple clusters withtap:qcow

Quoted 11 lines · Keir Fraser

Quoted 7 lines · Jake Wires
This patch is back to only allocating enough requests for one segment:
tools/blktap/drivers/block-qcow.c +2 -0
+        /* A segment (i.e. a page) can span multiple clusters */
+        s->max_aio_reqs = (getpagesize() / s->cluster_size) + 1;
In fact, this code allocates exactly two AIO requests for QCoW images created by qcow-create, which have a default cluster size of 4K.
What shall we do -- revert the whole patch or fix this line?

Goodness, words fail me. In all the confusion I created, that fix got
lost again. It should be:

+ s->max_aio_reqs = ((getpagesize() / s->cluster_size) + 1) * MAX_SEGMENTS_PER_REQ * MAX_REQUESTS;

Thanks,
Mark.

Mark McLoughlin

author

2007-05-03 17:54 UTC #8 lore raw

RE: [PATCH] Segments can span multiple clusters withtap:qcow

In reply to Jake Wires

Quoted 7 lines · Jake Wires

Hi,
This patch is back to only allocating enough requests for one segment:
tools/blktap/drivers/block-qcow.c +2 -0
+        /* A segment (i.e. a page) can span multiple clusters */
+        s->max_aio_reqs = (getpagesize() / s->cluster_size) + 1;

Quoted 7 lines

In fact, this code allocates exactly two AIO requests for QCoW images created by qcow-create, which have a default cluster size of 4K.
For a while now, tapdisk has supported EBUSY -- that is, if a plugin returns -EBUSY to tapdisk, tapdisk will put the last segment back on its queue and wait until the plugin has made progress before reissuing the request.

Yep.

Quoted 6 lines

Thus users should not observe an error when QCoW runs out of
AIO requests. This is attested by the fact that even with only 2 AIO requests allocated, QCoW block devices can handle a heavy load: I just mkfs'ed and copied a 1GB file to a QCoW image with no problem -- although it took quite a long while to do so, since only two segments were served at a time ;).

Uggh.

Quoted 3 lines

If you were observing errors while writing to QCoW devices, I'd like to know how you were causing them -- we may need to make some other changes to fix them. However, I'm not convinced that this patch is necessary.

I was seeing errors with the pre-EBUSY code, but I figured we would
still prefer to allocate the max number of segments.

Point taken that it's not critical, though. At this point, reverting
until after 3.1.0 wouldn't be crazy.

Thanks,
Mark.