[Qemu-devel] [PATCH] block/nfs: add knob to set readahead

[Qemu-devel] [PATCH] block/nfs: add knob to set readahead

[Peter Lieven]

upcoming libnfs will feature internal readahead support.
Add a knob to pass the optional readahead value as a URL
parameter.

This patch fixes also the incorrect usage of strncmp.

Signed-off-by: Peter Lieven <pl@kamp.de>
---
 block/nfs.c |   10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/block/nfs.c b/block/nfs.c
index ec43201..a5c0577 100644
--- a/block/nfs.c
+++ b/block/nfs.c
@@ -309,12 +309,16 @@ static int64_t nfs_client_open(NFSClient *client, const char *filename,
                        qp->p[i].name);
             goto fail;
         }
-        if (!strncmp(qp->p[i].name, "uid", 3)) {
+        if (!strcmp(qp->p[i].name, "uid")) {
             nfs_set_uid(client->context, atoi(qp->p[i].value));
-        } else if (!strncmp(qp->p[i].name, "gid", 3)) {
+        } else if (!strcmp(qp->p[i].name, "gid")) {
             nfs_set_gid(client->context, atoi(qp->p[i].value));
-        } else if (!strncmp(qp->p[i].name, "tcp-syncnt", 10)) {
+        } else if (!strcmp(qp->p[i].name, "tcp-syncnt")) {
             nfs_set_tcp_syncnt(client->context, atoi(qp->p[i].value));
+#ifdef LIBNFS_FEATURE_READAHEAD
+        } else if (!strcmp(qp->p[i].name, "readahead")) {
+            nfs_set_readahead(client->context, atoi(qp->p[i].value));
+#endif
         } else {
             error_setg(errp, "Unknown NFS parameter name: %s",
                        qp->p[i].name);
-- 
1.7.9.5

Re: [Qemu-devel] [PATCH] block/nfs: add knob to set readahead

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 2129 bytes --]

On 06/23/2014 08:30 AM, Peter Lieven wrote:
> upcoming libnfs will feature internal readahead support.
> Add a knob to pass the optional readahead value as a URL
> parameter.
> 
> This patch fixes also the incorrect usage of strncmp.

But not the incorrect usage of atoi() (hint - atoi cannot detect
overflow; it should NEVER be used on user-provided input; instead
strtol() should be preferred).

> 
> Signed-off-by: Peter Lieven <pl@kamp.de>
> ---
>  block/nfs.c |   10 +++++++---
>  1 file changed, 7 insertions(+), 3 deletions(-)
> 
> diff --git a/block/nfs.c b/block/nfs.c
> index ec43201..a5c0577 100644
> --- a/block/nfs.c
> +++ b/block/nfs.c
> @@ -309,12 +309,16 @@ static int64_t nfs_client_open(NFSClient *client, const char *filename,
>                         qp->p[i].name);
>              goto fail;
>          }
> -        if (!strncmp(qp->p[i].name, "uid", 3)) {
> +        if (!strcmp(qp->p[i].name, "uid")) {
>              nfs_set_uid(client->context, atoi(qp->p[i].value));

Of course, the use of atoi was pre-existing, so it doesn't necessarily
need to be done in _this_ patch.

> -        } else if (!strncmp(qp->p[i].name, "gid", 3)) {
> +        } else if (!strcmp(qp->p[i].name, "gid")) {
>              nfs_set_gid(client->context, atoi(qp->p[i].value));
> -        } else if (!strncmp(qp->p[i].name, "tcp-syncnt", 10)) {
> +        } else if (!strcmp(qp->p[i].name, "tcp-syncnt")) {
>              nfs_set_tcp_syncnt(client->context, atoi(qp->p[i].value));
> +#ifdef LIBNFS_FEATURE_READAHEAD
> +        } else if (!strcmp(qp->p[i].name, "readahead")) {
> +            nfs_set_readahead(client->context, atoi(qp->p[i].value));
> +#endif

I'm not a fan of adding even more special-casing to the file-name URI
without also adding it to the QAPI schema for use in the blockdev-add
command.  Of course, we don't have structured nfs options for
blockdev-add yet, but maybe it's time to start thinking about that
addition before we do this addition.

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH] block/nfs: add knob to set readahead

[Peter Lieven]

Am 23.06.2014 um 17:11 schrieb Eric Blake <eblake@redhat.com>:

> On 06/23/2014 08:30 AM, Peter Lieven wrote:
>> upcoming libnfs will feature internal readahead support.
>> Add a knob to pass the optional readahead value as a URL
>> parameter.
>> 
>> This patch fixes also the incorrect usage of strncmp.
> 
> But not the incorrect usage of atoi() (hint - atoi cannot detect
> overflow; it should NEVER be used on user-provided input; instead
> strtol() should be preferred).

Thanks for that hint I was not aware and somehow everybody
mist it when it first went in. I will respin for that.

> 
>> 
>> Signed-off-by: Peter Lieven <pl@kamp.de>
>> ---
>> block/nfs.c |   10 +++++++---
>> 1 file changed, 7 insertions(+), 3 deletions(-)
>> 
>> diff --git a/block/nfs.c b/block/nfs.c
>> index ec43201..a5c0577 100644
>> --- a/block/nfs.c
>> +++ b/block/nfs.c
>> @@ -309,12 +309,16 @@ static int64_t nfs_client_open(NFSClient *client, const char *filename,
>>                        qp->p[i].name);
>>             goto fail;
>>         }
>> -        if (!strncmp(qp->p[i].name, "uid", 3)) {
>> +        if (!strcmp(qp->p[i].name, "uid")) {
>>             nfs_set_uid(client->context, atoi(qp->p[i].value));
> 
> Of course, the use of atoi was pre-existing, so it doesn't necessarily
> need to be done in _this_ patch.
> 
>> -        } else if (!strncmp(qp->p[i].name, "gid", 3)) {
>> +        } else if (!strcmp(qp->p[i].name, "gid")) {
>>             nfs_set_gid(client->context, atoi(qp->p[i].value));
>> -        } else if (!strncmp(qp->p[i].name, "tcp-syncnt", 10)) {
>> +        } else if (!strcmp(qp->p[i].name, "tcp-syncnt")) {
>>             nfs_set_tcp_syncnt(client->context, atoi(qp->p[i].value));
>> +#ifdef LIBNFS_FEATURE_READAHEAD
>> +        } else if (!strcmp(qp->p[i].name, "readahead")) {
>> +            nfs_set_readahead(client->context, atoi(qp->p[i].value));
>> +#endif
> 
> I'm not a fan of adding even more special-casing to the file-name URI
> without also adding it to the QAPI schema for use in the blockdev-add
> command.  Of course, we don't have structured nfs options for
> blockdev-add yet, but maybe it's time to start thinking about that
> addition before we do this addition.

I would like to leave this for a follow-up and I promise to take care of this.
If you could give me a pointer of an existing driver that does this as a reference
I would be grateful.

For the read ahead parameter its likely not needed as (at least in my use case)
the read ahead makes only sense when converting a compressed QCOW2 Template File
which lives on an NFS Share to a RAW Device. So I use it with qemu-img.
For a Container File that is used as a VM hard drive I would likely not use read ahead
as the operating system itself will implement some sort of read ahead already.

Peter



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

Re: [Qemu-devel] [PATCH] block/nfs: add knob to set readahead

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 2095 bytes --]

On 06/23/2014 02:47 PM, Peter Lieven wrote:

>>> +#ifdef LIBNFS_FEATURE_READAHEAD
>>> +        } else if (!strcmp(qp->p[i].name, "readahead")) {
>>> +            nfs_set_readahead(client->context, atoi(qp->p[i].value));
>>> +#endif
>>
>> I'm not a fan of adding even more special-casing to the file-name URI
>> without also adding it to the QAPI schema for use in the blockdev-add
>> command.  Of course, we don't have structured nfs options for
>> blockdev-add yet, but maybe it's time to start thinking about that
>> addition before we do this addition.
> 
> I would like to leave this for a follow-up and I promise to take care of this.
> If you could give me a pointer of an existing driver that does this as a reference
> I would be grateful.

A good example of a recent conversion to structured options would be the
blkdebug and blkverify backends (look around commit 1bf20b82), or
proposed addition of new backends (such as archipelego,
https://lists.gnu.org/archive/html/qemu-devel/2014-06/msg05309.html).
It may be a bigger task than you anticipate, and certainly won't make
the 2.1 timeframe, but it's worth doing so if you're up to the task,
more power to you :)

> 
> For the read ahead parameter its likely not needed as (at least in my use case)
> the read ahead makes only sense when converting a compressed QCOW2 Template File
> which lives on an NFS Share to a RAW Device. So I use it with qemu-img.
> For a Container File that is used as a VM hard drive I would likely not use read ahead
> as the operating system itself will implement some sort of read ahead already.

Even if qemu-img is likely to be the only one ever specifying the
option, it still makes sense to expose it via QMP, as we are gradually
trying to move qemu-img over to more exclusive use of QMP (or at least
the underlying functions reached by QMP) rather than ad-hoc code.  For
example,
https://lists.gnu.org/archive/html/qemu-devel/2014-06/msg01822.html).

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH] block/nfs: add knob to set readahead

[Peter Lieven]

On 23.06.2014 23:05, Eric Blake wrote:
> On 06/23/2014 02:47 PM, Peter Lieven wrote:
>
>>>> +#ifdef LIBNFS_FEATURE_READAHEAD
>>>> +        } else if (!strcmp(qp->p[i].name, "readahead")) {
>>>> +            nfs_set_readahead(client->context, atoi(qp->p[i].value));
>>>> +#endif
>>> I'm not a fan of adding even more special-casing to the file-name URI
>>> without also adding it to the QAPI schema for use in the blockdev-add
>>> command.  Of course, we don't have structured nfs options for
>>> blockdev-add yet, but maybe it's time to start thinking about that
>>> addition before we do this addition.
>> I would like to leave this for a follow-up and I promise to take care of this.
>> If you could give me a pointer of an existing driver that does this as a reference
>> I would be grateful.
> A good example of a recent conversion to structured options would be the
> blkdebug and blkverify backends (look around commit 1bf20b82), or
> proposed addition of new backends (such as archipelego,
> https://lists.gnu.org/archive/html/qemu-devel/2014-06/msg05309.html).
> It may be a bigger task than you anticipate, and certainly won't make
> the 2.1 timeframe, but it's worth doing so if you're up to the task,
> more power to you :)
>
>> For the read ahead parameter its likely not needed as (at least in my use case)
>> the read ahead makes only sense when converting a compressed QCOW2 Template File
>> which lives on an NFS Share to a RAW Device. So I use it with qemu-img.
>> For a Container File that is used as a VM hard drive I would likely not use read ahead
>> as the operating system itself will implement some sort of read ahead already.
> Even if qemu-img is likely to be the only one ever specifying the
> option, it still makes sense to expose it via QMP, as we are gradually
> trying to move qemu-img over to more exclusive use of QMP (or at least
> the underlying functions reached by QMP) rather than ad-hoc code.  For
> example,
> https://lists.gnu.org/archive/html/qemu-devel/2014-06/msg01822.html).
>

Ok, it looks  like a bit of work. I will put it on my list for 2.2.

Thanks for the pointers,
Peter