Re: [Qemu-block] [Qemu-devel] [RFC PATCH 18/18] nbd: Implement NBD_CMD_WRITE_ZEROES on client

2016-04-09 Thread Alex Bligh 

On 10 Apr 2016, at 00:17, Eric Blake  wrote:

> No, the code is correct.  In both functions, the logic is that if the
> lower-level knows that the server respects FUA, then it clears the flag
> before returning (flags is passed by reference, not value).  Then at
> 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

Re: [Qemu-block] [Qemu-devel] [RFC PATCH 18/18] nbd: Implement NBD_CMD_WRITE_ZEROES on client

2016-04-09 Thread Eric Blake 
On 04/09/2016 04:57 AM, Alex Bligh wrote:
> 
> On 8 Apr 2016, at 23:05, Eric Blake  wrote:
> 
>> RFC because there is still discussion on the NBD list about
>> adding an NBD_OPT_ to let the client suggest server defaults
>> related to scanning for zeroes during NBD_CMD_WRITE, which may
>> tweak this patch.
>>
>> Upstream NBD protocol recently added the ability to efficiently
>> write zeroes without having to send the zeroes over the wire,
>> along with a flag to control whether the client wants a hole.
>>
>> The generic block code takes care of falling back to the obvious
>> write lots of zeroes if we return -ENOTSUP because the server
>> does not have WRITE_ZEROES.
>>

>> +ret = nbd_client_co_write_zeroes(bs, sector_num, nb_sectors, );
>> +if (ret < 0) {
>> +return ret;
>> +}
>> +
>> +/* The flag wasn't sent to the server, so we need to emulate it with an
>> + * explicit flush */
> 
> Surely you only need to do this is the flag wasn't sent to the server,
> i.e. if !(client->nbdflags & NBD_FLAG_SEND_FUA)
> 
> If you've sent a FUA request, no need to flush the whole thing.
> 
> nbd_co_writev_flags seems to have the same issue, which is where I guess
> you got that from.

No, the code is correct.  In both functions, the logic is that if the
lower-level knows that the server respects FUA, then it clears the flag
before returning (flags is passed by reference, not value).  Then at
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.

> 
>> +if (flags & BDRV_REQ_FUA) {
>> +ret = nbd_client_co_flush(bs);
>> +}
>> +

It's also this higher level that knows how to fall back to NBD_CMD_WRITE
if the lower-level returned -ENOTSUP because the server doesn't support
WRITE_ZEROES.

-- 
Eric Blake   eblake redhat com+1-919-301-3266
Libvirt virtualization library http://libvirt.org



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-block] [Qemu-devel] [RFC PATCH 18/18] nbd: Implement NBD_CMD_WRITE_ZEROES on client

2016-04-09 Thread Pavel Borzenkov 
On Sat, Apr 09, 2016 at 11:57:57AM +0100, Alex Bligh wrote:
> 
> On 8 Apr 2016, at 23:05, Eric Blake  wrote:
> 
> > RFC because there is still discussion on the NBD list about
> > adding an NBD_OPT_ to let the client suggest server defaults
> > related to scanning for zeroes during NBD_CMD_WRITE, which may
> > tweak this patch.
> > 
> > Upstream NBD protocol recently added the ability to efficiently
> > write zeroes without having to send the zeroes over the wire,
> > along with a flag to control whether the client wants a hole.
> > 
> > The generic block code takes care of falling back to the obvious
> > write lots of zeroes if we return -ENOTSUP because the server
> > does not have WRITE_ZEROES.
> > 
> > Signed-off-by: Eric Blake 
> > ---
> > block/nbd-client.h |  2 ++
> > block/nbd-client.c | 34 ++
> > block/nbd.c| 23 +++
> > 3 files changed, 59 insertions(+)
> > 
> > diff --git a/block/nbd-client.h b/block/nbd-client.h
> > index bc7aec0..2fe6654 100644
> > --- a/block/nbd-client.h
> > +++ b/block/nbd-client.h
> > @@ -47,6 +47,8 @@ void nbd_client_close(BlockDriverState *bs);
> > int nbd_client_co_discard(BlockDriverState *bs, int64_t sector_num,
> >   int nb_sectors);
> > int nbd_client_co_flush(BlockDriverState *bs);
> > +int nbd_client_co_write_zeroes(BlockDriverState *bs, int64_t sector_num,
> > +   int nb_sectors, int *flags);
> > int nbd_client_co_writev(BlockDriverState *bs, int64_t sector_num,
> >  int nb_sectors, QEMUIOVector *qiov, int *flags);
> > int nbd_client_co_readv(BlockDriverState *bs, int64_t sector_num,
> > diff --git a/block/nbd-client.c b/block/nbd-client.c
> > index f013084..4be83a8 100644
> > --- a/block/nbd-client.c
> > +++ b/block/nbd-client.c
> > @@ -291,6 +291,40 @@ int nbd_client_co_readv(BlockDriverState *bs, int64_t 
> > sector_num,
> > return nbd_co_readv_1(bs, sector_num, nb_sectors, qiov, offset);
> > }
> > 
> > +int nbd_client_co_write_zeroes(BlockDriverState *bs, int64_t sector_num,
> > +   int nb_sectors, int *flags)
> > +{
> > +ssize_t ret;
> > +NbdClientSession *client = nbd_get_client_session(bs);
> > +struct nbd_request request = { .type = NBD_CMD_WRITE_ZEROES };
> > +struct nbd_reply reply;
> > +
> > +if (!(client->nbdflags & NBD_FLAG_SEND_WRITE_ZEROES)) {
> > +return -ENOTSUP;
> > +}
> > +
> > +if ((*flags & BDRV_REQ_FUA) && (client->nbdflags & NBD_FLAG_SEND_FUA)) 
> > {
> > +*flags &= ~BDRV_REQ_FUA;
> > +request.flags |= NBD_CMD_FLAG_FUA;
> > +}
> > +if (!(*flags & BDRV_REQ_MAY_UNMAP)) {
> > +request.flags |= NBD_CMD_FLAG_NO_HOLE;
> > +}
> > +
> > +request.from = sector_num * 512;
> > +request.len = nb_sectors * 512;
> > +
> > +nbd_coroutine_start(client, );
> > +ret = nbd_co_send_request(bs, , NULL, 0);
> > +if (ret < 0) {
> > +reply.error = -ret;
> > +} else {
> > +nbd_co_receive_reply(client, , , NULL, 0);
> > +}
> > +nbd_coroutine_end(client, );
> > +return -reply.error;
> > +}
> > +
> > int nbd_client_co_writev(BlockDriverState *bs, int64_t sector_num,
> >  int nb_sectors, QEMUIOVector *qiov, int *flags)
> > {
> > diff --git a/block/nbd.c b/block/nbd.c
> > index f7ea3b3..f5119c0 100644
> > --- a/block/nbd.c
> > +++ b/block/nbd.c
> > @@ -355,6 +355,26 @@ static int nbd_co_readv(BlockDriverState *bs, int64_t 
> > sector_num,
> > return nbd_client_co_readv(bs, sector_num, nb_sectors, qiov);
> > }
> > 
> > +static int nbd_co_write_zeroes(BlockDriverState *bs, int64_t sector_num,
> > +   int nb_sectors, BdrvRequestFlags orig_flags)
> > +{
> > +int flags = orig_flags;
> > +int ret;
> > +
> > +ret = nbd_client_co_write_zeroes(bs, sector_num, nb_sectors, );
> > +if (ret < 0) {
> > +return ret;
> > +}
> > +
> > +/* The flag wasn't sent to the server, so we need to emulate it with an
> > + * explicit flush */
> 
> Surely you only need to do this is the flag wasn't sent to the server,
> i.e. if !(client->nbdflags & NBD_FLAG_SEND_FUA)
> 
> If you've sent a FUA request, no need to flush the whole thing.

In this case BDRV_REQ_FUA is cleared from 'flags' by
nbd_client_co_write_zeroes() and this condition becomes false.

> 
> nbd_co_writev_flags seems to have the same issue, which is where I guess
> you got that from.
> 
> > +if (flags & BDRV_REQ_FUA) {
> > +ret = nbd_client_co_flush(bs);
> > +}
> > +
> > +return ret;
> > +}
> > +
> > static int nbd_co_writev_flags(BlockDriverState *bs, int64_t sector_num,
> >int nb_sectors, QEMUIOVector *qiov, int 
> > flags)
> > {
> > @@ -476,6 +496,7 @@ static BlockDriver bdrv_nbd = {
> > .bdrv_parse_filename= nbd_parse_filename,
> > .bdrv_file_open

Re: [Qemu-block] [Qemu-devel] [RFC PATCH 18/18] nbd: Implement NBD_CMD_WRITE_ZEROES on client

2016-04-09 Thread Alex Bligh 

On 8 Apr 2016, at 23:05, Eric Blake  wrote:

> RFC because there is still discussion on the NBD list about
> adding an NBD_OPT_ to let the client suggest server defaults
> related to scanning for zeroes during NBD_CMD_WRITE, which may
> tweak this patch.
> 
> Upstream NBD protocol recently added the ability to efficiently
> write zeroes without having to send the zeroes over the wire,
> along with a flag to control whether the client wants a hole.
> 
> The generic block code takes care of falling back to the obvious
> write lots of zeroes if we return -ENOTSUP because the server
> does not have WRITE_ZEROES.
> 
> Signed-off-by: Eric Blake 
> ---
> block/nbd-client.h |  2 ++
> block/nbd-client.c | 34 ++
> block/nbd.c| 23 +++
> 3 files changed, 59 insertions(+)
> 
> diff --git a/block/nbd-client.h b/block/nbd-client.h
> index bc7aec0..2fe6654 100644
> --- a/block/nbd-client.h
> +++ b/block/nbd-client.h
> @@ -47,6 +47,8 @@ void nbd_client_close(BlockDriverState *bs);
> int nbd_client_co_discard(BlockDriverState *bs, int64_t sector_num,
>   int nb_sectors);
> int nbd_client_co_flush(BlockDriverState *bs);
> +int nbd_client_co_write_zeroes(BlockDriverState *bs, int64_t sector_num,
> +   int nb_sectors, int *flags);
> int nbd_client_co_writev(BlockDriverState *bs, int64_t sector_num,
>  int nb_sectors, QEMUIOVector *qiov, int *flags);
> int nbd_client_co_readv(BlockDriverState *bs, int64_t sector_num,
> diff --git a/block/nbd-client.c b/block/nbd-client.c
> index f013084..4be83a8 100644
> --- a/block/nbd-client.c
> +++ b/block/nbd-client.c
> @@ -291,6 +291,40 @@ int nbd_client_co_readv(BlockDriverState *bs, int64_t 
> sector_num,
> return nbd_co_readv_1(bs, sector_num, nb_sectors, qiov, offset);
> }
> 
> +int nbd_client_co_write_zeroes(BlockDriverState *bs, int64_t sector_num,
> +   int nb_sectors, int *flags)
> +{
> +ssize_t ret;
> +NbdClientSession *client = nbd_get_client_session(bs);
> +struct nbd_request request = { .type = NBD_CMD_WRITE_ZEROES };
> +struct nbd_reply reply;
> +
> +if (!(client->nbdflags & NBD_FLAG_SEND_WRITE_ZEROES)) {
> +return -ENOTSUP;
> +}
> +
> +if ((*flags & BDRV_REQ_FUA) && (client->nbdflags & NBD_FLAG_SEND_FUA)) {
> +*flags &= ~BDRV_REQ_FUA;
> +request.flags |= NBD_CMD_FLAG_FUA;
> +}
> +if (!(*flags & BDRV_REQ_MAY_UNMAP)) {
> +request.flags |= NBD_CMD_FLAG_NO_HOLE;
> +}
> +
> +request.from = sector_num * 512;
> +request.len = nb_sectors * 512;
> +
> +nbd_coroutine_start(client, );
> +ret = nbd_co_send_request(bs, , NULL, 0);
> +if (ret < 0) {
> +reply.error = -ret;
> +} else {
> +nbd_co_receive_reply(client, , , NULL, 0);
> +}
> +nbd_coroutine_end(client, );
> +return -reply.error;
> +}
> +
> int nbd_client_co_writev(BlockDriverState *bs, int64_t sector_num,
>  int nb_sectors, QEMUIOVector *qiov, int *flags)
> {
> diff --git a/block/nbd.c b/block/nbd.c
> index f7ea3b3..f5119c0 100644
> --- a/block/nbd.c
> +++ b/block/nbd.c
> @@ -355,6 +355,26 @@ static int nbd_co_readv(BlockDriverState *bs, int64_t 
> sector_num,
> return nbd_client_co_readv(bs, sector_num, nb_sectors, qiov);
> }
> 
> +static int nbd_co_write_zeroes(BlockDriverState *bs, int64_t sector_num,
> +   int nb_sectors, BdrvRequestFlags orig_flags)
> +{
> +int flags = orig_flags;
> +int ret;
> +
> +ret = nbd_client_co_write_zeroes(bs, sector_num, nb_sectors, );
> +if (ret < 0) {
> +return ret;
> +}
> +
> +/* The flag wasn't sent to the server, so we need to emulate it with an
> + * explicit flush */

Surely you only need to do this is the flag wasn't sent to the server,
i.e. if !(client->nbdflags & NBD_FLAG_SEND_FUA)

If you've sent a FUA request, no need to flush the whole thing.

nbd_co_writev_flags seems to have the same issue, which is where I guess
you got that from.

> +if (flags & BDRV_REQ_FUA) {
> +ret = nbd_client_co_flush(bs);
> +}
> +
> +return ret;
> +}
> +
> static int nbd_co_writev_flags(BlockDriverState *bs, int64_t sector_num,
>int nb_sectors, QEMUIOVector *qiov, int flags)
> {
> @@ -476,6 +496,7 @@ static BlockDriver bdrv_nbd = {
> .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,
> @@ -496,6 +517,7 @@ static BlockDriver bdrv_nbd_tcp = {
> .bdrv_parse_filename= nbd_parse_filename,
> .bdrv_file_open =