> On 10 Feb 2018, at 18:43, Alex Bligh <a...@alex.org.uk> wrote:
>
> So I think a reasonable logic for Qemu would be to try NBD_CMD_INFO and find
> the maximum write size, and if that's unsupported use 0x (capping at
> export size, or export size minus write
n't have a payload, I
think the rebuttal is that a server which supports NBD_CMD_WRITE of a given
length must also support NBD_CMD_WRITE_ZEROES of that length.
So I think a reasonable logic for Qemu would be to try NBD_CMD_INFO and find
the maximum write size, and if that's unsupported use 0x (capping at
export size, or export size minus write offset).
--
Alex Bligh
> On 14 Jan 2017, at 14:48, Wouter Verhelst <w...@uter.be> wrote:
>
> On Thu, Jan 12, 2017 at 06:56:42PM +0000, Alex Bligh wrote:
>> My preferred way to do this would be essentially to allow NBD_OPT_INFO
>> to be sent (wrapped appropriately) during the main transm
ize command) I guess it could
go into the NBD_OPT_INFO extension branch. If it's going to do
more than _INFO_, then I guess it needs its own extension.
--
Alex Bligh
signature.asc
Description: Message signed with OpenPGP using GPGMail
command,
let alone a resize command. The size of an NBD disk is (currently)
entirely in the hands of the server. What I think we'd really need
would be a 'reread size' command, and have the server capable of
supporting resizing. That would then work for readonly images too.
--
Alex Bligh
flags" field, where space is
> at a premium.
I did suggest a few non-Qemu uses (see below). I think it might be
an idea if the reference implementation supported it before
merging (which per below should be trivial).
--
Alex Bligh
> Begin forwarded message:
>
> From: Alex Bligh <a...
> On 6 Dec 2016, at 08:46, Alex Bligh <a...@alex.org.uk> wrote:
>
> I would support this.
>
> In fact the patch is sufficiently simple I think I'd merge this
> into extension-write-zeroes then merge that into master.
Hence:
Reviewed-By: Alex Bligh <a...@alex.org.uk>
--
Alex Bligh
nd I see no issue passing complete
zero status back to the client if it's so obvious from a stat().
--
Alex Bligh
___
> Nbd-general mailing list
> nbd-gene...@lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/nbd-general
>
--
Alex Bligh
> On 27 Sep 2016, at 00:41, Wouter Verhelst <w...@uter.be> wrote:
>
> Thoughts?
Honestly? This whole thing seems like complication for little gain. One command
per 2GB wiped doesn't seem like a bad thing.
--
Alex Bligh
ntire
disk easy to do by mistake!
This whole 'clear the whole disk in one command' seems like a
dire case of premature optimisation.
--
Alex Bligh
ize can't be either the minimum block size
or otherwise negotiated using NBD_OPT_INFO?
--
Alex Bligh
ise you not to use more than
2^31). And in any case, you probably want to parallelise reads and writes and
have more than one write in flight in any case, all of which suggests you are
going to be breaking up requests anyway.
--
Alex Bligh
> On 24 Sep 2016, at 18:13, Vladimir Sementsov-Ogievskiy
> <vsement...@virtuozzo.com> wrote:
>
> On 24.09.2016 19:49, Alex Bligh wrote:
>>> On 24 Sep 2016, at 17:42, Vladimir Sementsov-Ogievskiy
>>> <vsement...@virtuozzo.com> wrote:
>>>
>
> On 24 Sep 2016, at 17:52, Alex Bligh <a...@alex.org.uk> wrote:
>
> In *your* use-case holes may be desirable. However in the general case, you
> cannot assume a server supports holes. Optional support for holes isn't even
> in the mainline spec yet (AFAIR).
You
egy, you
cannot guarantee that the server will not implement them by ignoring
NBD_CMD_TRIM (this is perfectly acceptable as NBD_CMD_TRIM is advisory only)
and making NBD_CMD_WRITE_ZEREOS do a long write of zeroes.
IE there is no way to detect whether the server actually supports holes.
--
Alex Bligh
> On 24 Sep 2016, at 17:42, Vladimir Sementsov-Ogievskiy
> <vsement...@virtuozzo.com> wrote:
>
> On 24.09.2016 19:31, Alex Bligh wrote:
>>> On 24 Sep 2016, at 13:06, Vladimir Sementsov-Ogievskiy
>>> <vsement...@virtuozzo.com> wrote:
>>>
>
for a 'thick' write of zeroes on servers that don't support it.
--
Alex Bligh
size of the disk must be an exact
multiple of the minimum block size. So that would work.
--
Alex Bligh
limited to "whole disk" commands. We should probably make it an illegal
> flag for any command that actually sends data over the wire, though.
I'd prefer an approach like this. Perhaps X could be negotiated
with the block size extension (or simply be defined as the
'preferred block
uot; if we want to go down this route.
--
Alex Bligh
send a payload.
>
> In other words, the current behaviour of qemu is correct, is now
> documented to be correct, and should not be changed.
So what should those servers do (like 2 of mine) which don't buffer
the entire read, if they get an error having already sent some data?
--
A
en reasonably well known (I wrote about it
at least 3 years ago), that the current implementation
(reference + kernel) does not cope well with errors
on reads, so I'm guessing one is just trading one
set of brokenness for another. So I'm pretty relaxed
about what goes in qemu.
--
Alex Bligh
On 14 Jun 2016, at 14:32, Paolo Bonzini <pbonz...@redhat.com> wrote:
>
> On 13/06/2016 23:41, Alex Bligh wrote:
>> That's one of the reasons that there is a proposal to add
>> STRUCTURED_READ to the spec (although I still haven't had time to
>> implement that for
ub.com/yoe/nbd/blob/master/nbd-server.c#L1734
In essence read error handling is a horrible mess in NBD, and
I would not expect it to work in general :-(
--
Alex Bligh
signature.asc
Description: Message signed with OpenPGP using GPGMail
k good to me.
--
Alex Bligh
On 11 May 2016, at 22:12, Wouter Verhelst <w...@uter.be> wrote:
> On Tue, May 10, 2016 at 04:38:29PM +0100, Alex Bligh wrote:
>> On 10 May 2016, at 16:29, Eric Blake <ebl...@redhat.com> wrote:
>>> So the kernel is currently one of the clients that
kernel needs to do at least
some breaking up.
--
Alex Bligh
(1) with -p ?
http://man7.org/linux/man-pages/man1/fallocate.1.html
As it takes length and offset in TB, PB, EB, ZB and YB, it
seems to be 64 bit aware :-)
--
Alex Bligh
signature.asc
Description: Message signed with OpenPGP using GPGMail
ize constraints or not, it is
going to need to do *some* breaking up of requests.
--
Alex Bligh
signature.asc
Description: Message signed with OpenPGP using GPGMail
ther way?
>
> No extension in play. The kernel is obeying NBD_FLAG_SEND_TRIM, which
> is in the normative standard, and unrelated to the INFO/BLOCK_SIZE
> extensions.
My mistake. I was confusing 'WRITE_ZEROES' with 'TRIM'.
--
Alex Bligh
signature.asc
Description: Message signed with OpenPGP using GPGMail
-
> Mobile security can be enabling, not merely restricting. Employees who
> bring their own devices (BYOD) to work are irked by the imposition of MDM
> restrictions. Mobile Device Manager Plus allows you to control only the
> apps on BYO-devices by containerizing them, leaving personal data untouched!
> https://ad.doubleclick.net/ddm/clk/304595813;131938128;j___
> Nbd-general mailing list
> nbd-gene...@lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/nbd-general
--
Alex Bligh
signature.asc
Description: Message signed with OpenPGP using GPGMail
r servers.
I'm going to find a minimal patch to nbd-client and offer that
to Ubuntu / Debian.
This message is here in part so I have something to point them at
on the mailing list :-)
--
Alex Bligh
it won't play
nicely with all the other stuff you've been doing).
--
Alex Bligh
signature.asc
Description: Message signed with OpenPGP using GPGMail
> info->size);
> return -E2BIG;
> }
> @@ -724,18 +784,18 @@ int nbd_init(int fd, QIOChannelSocket *sioc,
> NbdExportInfo *info)
> return -serrno;
> }
>
> -TRACE("Setting block size to %lu", (unsigned long)BDRV_SECTOR_SIZE);
> +TRACE("Setting block size to %lu", sector_size);
>
> -if (ioctl(fd, NBD_SET_BLKSIZE, (unsigned long)BDRV_SECTOR_SIZE) < 0) {
> +if (ioctl(fd, NBD_SET_BLKSIZE, sector_size) < 0) {
> int serrno = errno;
> LOG("Failed setting NBD block size");
> return -serrno;
> }
>
> TRACE("Setting size to %lu block(s)", sectors);
> -if (size % BDRV_SECTOR_SIZE) {
> -TRACE("Ignoring trailing %d bytes of export",
> - (int) (size % BDRV_SECTOR_SIZE));
> +if (info->size % sector_size) {
> +TRACE("Ignoring trailing %" PRId64 " bytes of export",
> + info->size % sector_size);
> }
>
> if (ioctl(fd, NBD_SET_SIZE_BLOCKS, sectors) < 0) {
> --
> 2.5.5
>
>
--
Alex Bligh
block code takes care of falling back to the obvious
> write of lots of zeroes if we return -ENOTSUP because the server
> does not have WRITE_ZEROES.
>
> Signed-off-by: Eric Blake <ebl...@redhat.com>
Reviewed-by: Alex Bligh <a...@alex.org.uk>
>
> ---
> v3: rebase
f-by: Eric Blake <ebl...@redhat.com>
Reviewed-by: Alex Bligh <a...@alex.org.uk>
>
> ---
> v3: abandon NBD_CMD_CLOSE extension, rebase to use blk_pwrite_zeroes
> ---
> include/block/nbd.h | 7 +--
> nbd/server.c| 42
es (including when a server
> requires TLS but does not have NBD_OPT_GO!), and on success
> it provides at least as much info as NBD_OPT_EXPORT_NAME sends.
>
> Signed-off-by: Eric Blake <ebl...@redhat.com>
>
Reviewed-by: Alex Bligh <a...@alex.org.uk>
> ---
>
case NBD_OPT_ABORT:
> -return -EINVAL;
> + /* NBD spec says we must reply before disconnecting,
> + * but that we must also tolerate guests that don't
> + * wait for our reply. */
> +ret = nbd_negotiate_send_rep(client->ioc, NBD_REP_ACK,
> + clientflags);
> +if (!ret) {
> +ret = -EINVAL;
> +}
> +return ret;
>
> case NBD_OPT_EXPORT_NAME:
> return nbd_negotiate_handle_export_name(client, length);
> --
> 2.5.5
>
>
--
Alex Bligh
-off-by: Eric Blake <ebl...@redhat.com>
Reviewed-by: Alex Bligh <a...@alex.org.uk>
>
> ---
> v3: rebase to changes earlier in series
> ---
> nbd/server.c | 48 +++-
> 1 file changed, 23 insertions(+), 25 deletions(-)
>
&
> Signed-off-by: Eric Blake <ebl...@redhat.com>
> Reviewed-by: Alex Bligh <a...@alex.org.uk>
>
> ---
> v3: rebase, tweak a debug message
> ---
> include/block/nbd.h | 29 +-
> nbd/nbd-internal.h | 2 +-
> nbd/client.c| 250 +++
nts in nbd.h based on the current upstream
> NBD protocol (https://github.com/yoe/nbd/blob/master/doc/proto.md),
> and touch some nearby code to keep checkpatch.pl happy.
>
> Signed-off-by: Eric Blake <ebl...@redhat.com>
Reviewed-by: Alex Bligh <a...@alex.org.uk>
>
> ---
1024). So for now, just
> stick to the required minimum.
>
> Signed-off-by: Eric Blake <ebl...@redhat.com>
Reviewed-by: Alex Bligh <a...@alex.org.uk>
>
> ---
> v3: enlarge the limit, and document choice of new value
> ---
> include/block/nbd.h | 6 ++
> nbd/cl
t; -goto out;
> +/* unreachable, thanks to special case in nbd_co_receive_request() */
> +abort();
> +
> case NBD_CMD_FLUSH:
> TRACE("Request type is FLUSH");
>
> @@ -1182,10 +1203,14 @@ static void nbd_trip(void *opaque)
> break;
> default:
> LOG("invalid request type (%" PRIu32 ") received", request.type);
> -invalid_request:
> reply.error = EINVAL;
> error_reply:
> -if (nbd_co_send_reply(req, , 0) < 0) {
> +/* We must disconnect after replying with an error to
> + * NBD_CMD_READ, since we choose not to send bogus filler
> + * data; likewise after NBD_CMD_WRITE if we did not read the
> + * payload. */
> +if (nbd_co_send_reply(req, , 0) < 0 || command == NBD_CMD_READ
> ||
> +(command == NBD_CMD_WRITE && !req->complete)) {
> goto out;
> }
> break;
> --
> 2.5.5
>
>
--
Alex Bligh
also makes it easier to test commit 200650d4, which
> is the client counterpart of receiving the description.
>
> Signed-off-by: Eric Blake <ebl...@redhat.com>
Reviewed-by: Alex Bligh <a...@alex.org.uk>
> ---
> include/block/nbd.h | 1 +
> nbd/nb
!=
> @@ -806,7 +804,7 @@ static void nbd_eject_notifier(Notifier *n, void *data)
> }
>
> NBDExport *nbd_export_new(BlockBackend *blk, off_t dev_offset, off_t size,
> - uint32_t nbdflags, void (*close)(NBDExport *),
> + uint16_t nbdflags, void (*close)(NBDExport *),
> Error **errp)
> {
> NBDExport *exp = g_malloc0(sizeof(NBDExport));
> diff --git a/qemu-nbd.c b/qemu-nbd.c
> index 2c9754e..71bfdeb 100644
> --- a/qemu-nbd.c
> +++ b/qemu-nbd.c
> @@ -241,7 +241,7 @@ static void *nbd_client_thread(void *arg)
> {
> char *device = arg;
> off_t size;
> -uint32_t nbdflags;
> +uint16_t nbdflags;
> QIOChannelSocket *sioc;
> int fd;
> int ret;
> @@ -455,7 +455,7 @@ int main(int argc, char **argv)
> BlockBackend *blk;
> BlockDriverState *bs;
> off_t dev_offset = 0;
> -uint32_t nbdflags = 0;
> +uint16_t nbdflags = 0;
> bool disconnect = false;
> const char *bindto = "0.0.0.0";
> const char *port = NULL;
> --
> 2.5.5
>
>
--
Alex Bligh
Sanity check things before blindly reading incorrectly.
>>
>> Signed-off-by: Eric Blake <ebl...@redhat.com>
Reviewed-by: Alex Bligh <a...@alex.org.uk>
>> ---
>>
>> Yet another case of code introduced in 2.6 that doesn't play
>> nicely with spec-compliant se
e trying, on the grounds that maybe the client will get the
>> hint to send NBD_OPT_STARTTLS.
>>
>> Signed-off-by: Eric Blake <ebl...@redhat.com>
Reviewed-by: Alex Bligh <a...@alex.org.uk>
It is worth noting that this change (assuming I've read
it right) in no way means
r with openssl and would be concerned that gnutls_bye()
might block.
--
Alex Bligh
signature.asc
Description: Message signed with OpenPGP using GPGMail
t
> this higher level, if FUA is still set, the server is too old, so we do
> a fallback flush to get the same semantics for the write in question,
> but at higher cost of a full flush.
OK, thanks.
--
Alex Bligh
signature.asc
Description: Message signed with OpenPGP using GPGMail
= BDRV_REQ_FUA,
> @@ -516,6 +538,7 @@ static BlockDriver bdrv_nbd_unix = {
> .bdrv_parse_filename= nbd_parse_filename,
> .bdrv_file_open = nbd_open,
> .bdrv_co_readv = nbd_co_readv,
> +.bdrv_co_write_zeroes = nbd_co_write_zeroes,
> .bdrv_co_writev = nbd_co_writev,
> .bdrv_co_writev_flags = nbd_co_writev_flags,
> .supported_write_flags = BDRV_REQ_FUA,
> --
> 2.5.5
>
>
--
Alex Bligh
LOG("writing to file failed");
> +reply.error = -ret;
> +goto error_reply;
> +}
> +
> +/* FIXME: do we need FUA flush here, if we also passed it to
> + * blk_write_zeroes? */
I don't think so. blk_write_zeroes with BDRV_REQ_FUA should
make *that* request access the media, but FLUSH makes ALL PREVIOUS
requests access the media, so you are doing too much work here
I think.
> +if (request.flags & NBD_CMD_FLAG_FUA) {
> +ret = blk_co_flush(exp->blk);
> +if (ret < 0) {
> +LOG("flush failed");
> +reply.error = -ret;
> +goto error_reply;
> +}
> +}
> +
> +if (nbd_co_send_reply(req, , 0) < 0) {
> +goto out;
> +}
> +break;
> case NBD_CMD_DISC:
> TRACE("Request type is DISCONNECT");
> goto out;
> --
> 2.5.5
>
>
--
Alex Bligh
e NBD_CMD_CLOSE:
> +TRACE("Request type is CLOSE");
> +if (request.flags || request.from || request.len) {
> +LOG("bad parameters, skipping flush");
> +reply.error = EINVAL;
> +} else {
> +ret = blk_co_flush(exp->blk);
> +if (ret < 0) {
> +LOG("flush failed");
> +reply.error = -ret;
> +}
> +}
> +/* Attempt to send reply, but even if it fails, we are done */
> +nbd_co_send_reply(req, , 0);
> goto out;
> case NBD_CMD_FLUSH:
> TRACE("Request type is FLUSH");
> --
> 2.5.5
>
>
--
Alex Bligh
D_FLAG_C_NO_ZEROES (only possible in
> newstyle, whether or not it is fixed newstyle). It doesn't
> shave much off the wire, but we might as well implement it.
>
> Signed-off-by: Eric Blake <ebl...@redhat.com>
Reviewed-by: Alex Bligh <a...@alex.org.uk>
thanks - that was
> Signed-off-by: Eric Blake <ebl...@redhat.com>
Reviewed-by: Alex Bligh <a...@alex.org.uk>
> ---
> include/block/nbd.h | 29 +-
> nbd/nbd-internal.h | 2 +-
> nbd/client.c| 250 ++--
> 3 files changed,
> is 16 bits in response to NBD_OPT_EXPORT_NAME and NBD_OPT_GO.
>
> Signed-off-by: Eric Blake <ebl...@redhat.com>
Reviewed-by: Alex Bligh <a...@alex.org.uk>
> ---
> include/block/nbd.h | 2 +-
> nbd/server.c| 10 --
> qemu-nbd.c | 2 +-
> 3 f
nts in nbd.h based on the current upstream
> NBD protocol (https://github.com/yoe/nbd/blob/master/doc/proto.md),
> and touch some nearby code to keep checkpatch.pl happy.
>
> Signed-off-by: Eric Blake <ebl...@redhat.com>
Reviewed-by: Alex Bligh <a...@alex.org.uk>
> ---
> i
tch is merged) document the
maximum supported export length as 4096, why not change
this to 4096?
Otherwise:
Reviewed-by: Alex Bligh <a...@alex.org.uk>
>
> ssize_t nbd_wr_syncv(QIOChannel *ioc,
> struct iovec *iov,
> diff --git a/nbd/client.c b/nbd/client.c
&g
efore initializing anything else.
>
> This is important because some versions of gnutls/gcrypt
> require explicit initialization before use.
>
> Signed-off-by: Daniel P. Berrange <berra...@redhat.com>
Reviewed-by: Alex Bligh <a...@alex.org.uk>
Tested-by: Alex Bli
59 matches
Mail list logo
