search for: req_on

On Fri, Mar 08, 2019 at 10:50:35AM -0600, Eric Blake wrote: > > int extents (void *handle, uint32_t count, uint64_t offset, > > uint32_t flags /* always 0? */, > > Unless we support the REQ_ONE flag (where the client specifically wants > only one extent returned). ... > > nbdkit would need to do some massaging on this to get it into the > > right format for NBD_CMD_BLOCK_STATUS. (I'm very confused about what > > NBD_CMD_FLAG_REQ_ONE is supposed to do.) > &gt...

...1/19 2:44 PM, Richard W.M. Jones wrote: >> On Fri, Mar 08, 2019 at 10:50:35AM -0600, Eric Blake wrote: >>>> int extents (void *handle, uint32_t count, uint64_t offset, >>>> uint32_t flags /* always 0? */, >>> >>> Unless we support the REQ_ONE flag (where the client specifically wants >>> only one extent returned). >> ... >>>> nbdkit would need to do some massaging on this to get it into the >>>> right format for NBD_CMD_BLOCK_STATUS. (I'm very confused about what >>>> NBD_CMD_FLAG...

...+++++++++++++++++++++++ > 2 files changed, 152 insertions(+), 1 deletion(-) > > +static int > +vddk_extents (void *handle, uint32_t count, uint64_t offset, uint32_t flags, > + struct nbdkit_extents *extents) > +{ > + struct vddk_handle *h = handle; > + bool req_one = flags & NBDKIT_FLAG_REQ_ONE; > + uint64_t position, end, start_sector; > + > + position = offset; > + ... > + > + for (i = 0; i < block_list->numBlocks; ++i) { > + uint64_t offset, length; > + > + offset = block_list->blocks[i].offset * VIXD...

...W.M. Jones wrote: > >>On Fri, Mar 08, 2019 at 10:50:35AM -0600, Eric Blake wrote: > >>>> int extents (void *handle, uint32_t count, uint64_t offset, > >>>> uint32_t flags /* always 0? */, > >>> > >>>Unless we support the REQ_ONE flag (where the client specifically wants > >>>only one extent returned). > >>... > >>>>nbdkit would need to do some massaging on this to get it into the > >>>>right format for NBD_CMD_BLOCK_STATUS. (I'm very confused about what > >>...

On 3/11/19 2:44 PM, Richard W.M. Jones wrote: > On Fri, Mar 08, 2019 at 10:50:35AM -0600, Eric Blake wrote: >>> int extents (void *handle, uint32_t count, uint64_t offset, >>> uint32_t flags /* always 0? */, >> >> Unless we support the REQ_ONE flag (where the client specifically wants >> only one extent returned). > ... >>> nbdkit would need to do some massaging on this to get it into the >>> right format for NBD_CMD_BLOCK_STATUS. (I'm very confused about what >>> NBD_CMD_FLAG_REQ_ONE is supposed...

...>> NB During this I discovered another (but minor) bug in qemu. If you >>> feed qemu a long series of block status replies then it eventually >>> closes the connection. However it does not print an error message. >>> >> >> Which version(s) of qemu? Was REQ_ONE in force or not? Was it a long >> series of status replies that could have been coalesced, or were they >> alternating status? Did the replies extend beyond the client's original >> request? >> >> I'd like to reproduce the failure, to determine if qemu needs m...

..._list"); > > > + VixDiskLib_FreeBlockList (block_list); > > > + return -1; > > > + } > > > + > > > + position = offset + length; > > > > This inner loop might be long; you could add this: > > > > if (req_one) > > break; So interestingly _no_ you can't add that here, and the reason does reveal why the REQ_ONE flag is still hard to get right even with the simpler implementation of extents. Because VDDK's QueryAllocatedBlocks API only works for some very large aligned block size (64K II...

I pushed this one already. It should go into the stable-1.12 branch too. Rich.

...n easier), and adjacent extents of the same type would be coalesced automatically. But it's an error if returned extents overlap each other. nbdkit would need to do some massaging on this to get it into the right format for NBD_CMD_BLOCK_STATUS. (I'm very confused about what NBD_CMD_FLAG_REQ_ONE is supposed to do.) We will also need a corresponding ‘can_extents’, which is analogous to ‘can_write’ etc and is what would control the output of NBD_OPT_{SET,LIST}_META_CONTEXT. For nbdkit-file-filter: - Fairly simple implementation using SEEK_HOLE/SEEK_DATA. - Not sure how we detect zeroes...

...> +#include <errno.h> #include <libnbd.h> @@ -30,21 +32,31 @@ static const char *unixsocket; static const char *bitmap; static const char *base_allocation = "base:allocation"; -static int calls; /* Track which contexts passed through callback */ +struct data { + bool req_one; /* input: true if req_one was passed to request */ + int count; /* input: count of expected remaining calls */ + bool fail; /* input: true to return failure */ + bool seen_base; /* output: true if base:allocation encountered */ + bool seen_dirty; /* output: true if qemu:dirty-...

On Thu, May 16, 2019 at 08:05:18AM -0500, Eric Blake wrote: >On 5/15/19 5:39 PM, Martin Kletzander wrote: >> This filter caches the last result of the extents() call and offers a nice >> speed-up for clients that only support req_on=1 in combination with plugins like > >s/on/one/ > >> vddk, which has no overhead for returning information for multiple extents in >> one call, but that call is very time-consuming. >> >> Quick test showed that on a fast connection and a sparsely allocated 16G disk...

...that patch for that issue already). > > NB During this I discovered another (but minor) bug in qemu. If you > feed qemu a long series of block status replies then it eventually > closes the connection. However it does not print an error message. > Which version(s) of qemu? Was REQ_ONE in force or not? Was it a long series of status replies that could have been coalesced, or were they alternating status? Did the replies extend beyond the client's original request? I'd like to reproduce the failure, to determine if qemu needs more patches. -- Eric Blake, Principal Soft...

Rather than moving the connection to DEAD, we can just ignore further contexts to the existing command handle, and fail the overall command with the same errno as the failed callback. Eric Blake (2): states: Track cmd->error as errno, not wire value api: Recover from block status callback failure generator/generator | 5 ++- generator/states-reply-simple.c | 2 +-

..._id; > + size_t i; > + int r; > + > + assert (cmd == NBD_CMD_BLOCK_STATUS); > + Worth asserting conn->meta_context_base_allocation? > + > + /* Send each block descriptor. */ > + for (i = 0; i < nr_extents; ++i) { Where does the list terminate after 1 extent if REQ_ONE was set? Also, if REQ_ONE is set but the plugin provided coalesced status beyond the request, this would need to clamp the answer to the requested length. > + const struct nbdkit_extent e = nbdkit_get_extent (extents, i); > + struct block_descriptor bd; > + > + if (i == 0) &g...

...t result when talking to a client that did not negotiate 64-bit commands) > uint32_t flag; // hole, zero, data ... more in future? > }; > > int extents (void *handle, uint32_t count, uint64_t offset, > uint32_t flags /* always 0? */, Unless we support the REQ_ONE flag (where the client specifically wants only one extent returned). > size_t *nr_extents, struct nbdkit_extent *extents); > > The function is meant to scan [offset, offset+count-1] and return a > list of all extents overlapping this range, and their status (hole, >...

...("total size=%" PRIu64, h->size); @@ -319,6 +341,104 @@ split_cache (void *handle, uint32_t count, uint64_t offset, uint32_t flags) } #endif /* HAVE_POSIX_FADVISE */ +#ifdef SEEK_HOLE +static int64_t +do_extents (struct file *file, uint32_t count, uint64_t offset, + bool req_one, struct nbdkit_extents *extents) +{ + int64_t r = 0; + uint64_t end = offset + count; + + do { + off_t pos; + + pos = lseek (file->fd, offset, SEEK_DATA); + if (pos == -1) { + if (errno == ENXIO) { + /* The current man page does not describe this situation well, +...

This filter caches the last result of the extents() call and offers a nice speed-up for clients that only support req_one=1 in combination with plugins like vddk, which has no overhead for returning information for multiple extents in one call, but that call is very time-consuming. Quick test showed that on a fast connection and a sparsely allocated 16G disk with a OS installed `qemu-img map` runs 16s instead of 33s...

...4/23/19 4:49 PM, Richard W.M. Jones wrote: > > >> > >> ...to here, after the final nbdkit_add_extent, so that we can return a > >> larger extent than the client's request. I remember when I originally > >> asked, you declined due to odd interactions with REQ_ONE semantics, but > >> since then, we changed how add_extent() works. Does it work now to defer > >> the clamping? > > > > It's a bit late at night for me to think clearly about extents, but > > here is a description of the original problem with moving the cla...

On 4/23/19 4:49 PM, Richard W.M. Jones wrote: >> >> ...to here, after the final nbdkit_add_extent, so that we can return a >> larger extent than the client's request. I remember when I originally >> asked, you declined due to odd interactions with REQ_ONE semantics, but >> since then, we changed how add_extent() works. Does it work now to defer >> the clamping? > > It's a bit late at night for me to think clearly about extents, but > here is a description of the original problem with moving the clamp: > > https://ww...

...t;VixDiskLib_FreeBlockList", "block_list"); > > + VixDiskLib_FreeBlockList (block_list); > > + return -1; > > + } > > + > > + position = offset + length; > > This inner loop might be long; you could add this: > > if (req_one) > break; The issue in practice is that VixDiskLib_QueryAllocatedBlocks is very expensive (because it's a remote procedure call), but we expect calling nbdkit_add_extent should be very cheap. However I'll experiment with breaking earlier from the inner loop. I need to do a lot more...