Patchwork [2/3] block-nbd: fix use of protocols in backing files and nbd probing

login
register
mail settings
Submitter Anthony Liguori
Date Sept. 11, 2010, 2:04 p.m.
Message ID <1284213896-12705-3-git-send-email-aliguori@us.ibm.com>
Download mbox | patch
Permalink /patch/64514/
State New
Headers show

Comments

Anthony Liguori - Sept. 11, 2010, 2:04 p.m.
The use of protocols in backing_files is currently broken because of some
checks for adjusting relative pathnames.

Additionally, there's a spurious read when using an nbd protocol that can be
quite destructive when using copy-on-read.  Potentially, this can lead to
probing an image file over top of NBD but this is completely wrong as NBD
devices are not growable.

Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
---
NB: this is absolutely not ideal.  A more elegant suggestion would be
appreciated.  I don't think NBD cleanly fits the model of a protocol as it
stands today.
Stefan Hajnoczi - Sept. 11, 2010, 4:53 p.m.
On Sat, Sep 11, 2010 at 3:04 PM, Anthony Liguori <aliguori@us.ibm.com> wrote:
> Additionally, there's a spurious read when using an nbd protocol that can be
> quite destructive when using copy-on-read.  Potentially, this can lead to
> probing an image file over top of NBD but this is completely wrong as NBD
> devices are not growable.

Can you describe the copy-on-read scenario where the 2 KB probe read
is a problem?

Accessing a fixed size image file over NBD is probably uncommon, but
I'm not sure if there's a reason to forbid it.

Stefan
Anthony Liguori - Sept. 11, 2010, 5:27 p.m.
On 09/11/2010 11:53 AM, Stefan Hajnoczi wrote:
> On Sat, Sep 11, 2010 at 3:04 PM, Anthony Liguori<aliguori@us.ibm.com>  wrote:
>    
>> Additionally, there's a spurious read when using an nbd protocol that can be
>> quite destructive when using copy-on-read.  Potentially, this can lead to
>> probing an image file over top of NBD but this is completely wrong as NBD
>> devices are not growable.
>>      
> Can you describe the copy-on-read scenario where the 2 KB probe read
> is a problem?
>
> Accessing a fixed size image file over NBD is probably uncommon, but
> I'm not sure if there's a reason to forbid it.
>    

I think the better solution is to explicitly specific raw with nbd.  
IOW, I think -drive file=nbd:localhost:1026,format=raw should work the 
same way.  I still feel slightly weird about probing happening with 
nbd.  It seems like it could only result in badness.

The specific scenario is migration.  I'm using a copy-on-read file on 
the destination and I want to be sure that I don't read any blocks 
(since they're copied) until the source stops execution.

Regards,

Anthony Liguori

> Stefan
>
>
Anthony Liguori - Sept. 11, 2010, 5:45 p.m.
On 09/11/2010 12:27 PM, Anthony Liguori wrote:
> I think the better solution is to explicitly specific raw with nbd.  
> IOW, I think -drive file=nbd:localhost:1026,format=raw should work the 
> same way.  I still feel slightly weird about probing happening with 
> nbd.  It seems like it could only result in badness.

Yeah, w/o this patch, 'qemu-img create -f qcow2 -b nbd:localhost:1026 -o 
backing_fmt=raw foo.img' doesn't generate any reads although if you drop 
-o backing_fmt, it will.

Not sure I agree it's the most useful thing, but I'm not going to claim 
it's never useful so this patch is not a good idea.

Regards,

Anthony Liguori

>
> The specific scenario is migration.  I'm using a copy-on-read file on 
> the destination and I want to be sure that I don't read any blocks 
> (since they're copied) until the source stops execution.
>
> Regards,
>
> Anthony Liguori
>
>> Stefan
>>
>
Juan Quintela - Sept. 15, 2010, 4:06 p.m.
Anthony Liguori <aliguori@us.ibm.com> wrote:
> The use of protocols in backing_files is currently broken because of some
> checks for adjusting relative pathnames.
>
> Additionally, there's a spurious read when using an nbd protocol that can be
> quite destructive when using copy-on-read.  Potentially, this can lead to
> probing an image file over top of NBD but this is completely wrong as NBD
> devices are not growable.
>
> Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
> ---
> NB: this is absolutely not ideal.  A more elegant suggestion would be
> appreciated.  I don't think NBD cleanly fits the model of a protocol as it
> stands today.

Bad, bad boy, you fixed two things in a single patch.

>
> diff --git a/block.c b/block.c
> index cd2ee31..a32d5dd 100644
> --- a/block.c
> +++ b/block.c
> @@ -344,6 +344,12 @@ static int find_image_format(const char *filename, BlockDriver **pdrv)
>          return ret;
>      }
>  
> +    if (strcmp(bs->drv->protocol_name, "nbd") == 0) {
> +        drv = bs->drv;
> +        bdrv_delete(bs);
> +        goto out;
> +    }
> +
>      /* Return the raw BlockDriver * to scsi-generic devices or empty drives */
>      if (bs->sg || !bdrv_is_inserted(bs)) {
>          bdrv_delete(bs);
> @@ -373,6 +379,7 @@ static int find_image_format(const char *filename, BlockDriver **pdrv)
>              }
>          }
>      }
> +out:
>      if (!drv) {
>          ret = -ENOENT;
>      }

I have no opinion about this change.

> @@ -603,10 +610,16 @@ int bdrv_open(BlockDriverState *bs, const char *filename, int flags,
>          BlockDriver *back_drv = NULL;
>  
>          bs->backing_hd = bdrv_new("");
> -        path_combine(backing_filename, sizeof(backing_filename),
> -                     filename, bs->backing_file);
> -        if (bs->backing_format[0] != '\0')
> -            back_drv = bdrv_find_format(bs->backing_format);
> +        back_drv = bdrv_find_protocol(bs->backing_file);
> +        if (!back_drv) {
> +            path_combine(backing_filename, sizeof(backing_filename),
> +                         filename, bs->backing_file);
> +            if (bs->backing_format[0] != '\0')
> +                back_drv = bdrv_find_format(bs->backing_format);
> +        } else {
> +            pstrcpy(backing_filename, sizeof(backing_filename),
> +                    bs->backing_file);
> +        }
>  
>          /* backing files always opened read-only */
>          back_flags =

But this one breaks my setup, I have to backout this patch to be able to
launch guests with qcow2 file images.

Later, Juan.
Kevin Wolf - Sept. 16, 2010, 8:08 a.m.
Am 11.09.2010 16:04, schrieb Anthony Liguori:
> The use of protocols in backing_files is currently broken because of some
> checks for adjusting relative pathnames.
> 
> Additionally, there's a spurious read when using an nbd protocol that can be
> quite destructive when using copy-on-read.  Potentially, this can lead to
> probing an image file over top of NBD but this is completely wrong as NBD
> devices are not growable.
> 
> Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
> ---
> NB: this is absolutely not ideal.  A more elegant suggestion would be
> appreciated.  I don't think NBD cleanly fits the model of a protocol as it
> stands today.
> 
> diff --git a/block.c b/block.c
> index cd2ee31..a32d5dd 100644
> --- a/block.c
> +++ b/block.c
> @@ -344,6 +344,12 @@ static int find_image_format(const char *filename, BlockDriver **pdrv)
>          return ret;
>      }
>  
> +    if (strcmp(bs->drv->protocol_name, "nbd") == 0) {
> +        drv = bs->drv;
> +        bdrv_delete(bs);
> +        goto out;
> +    }

Is nbd really the only protocol that behaves like this? I don't like
hardcoding driver names in generic block layer code.

Kevin
Anthony Liguori - Sept. 16, 2010, 1 p.m.
On 09/16/2010 03:08 AM, Kevin Wolf wrote:
> Am 11.09.2010 16:04, schrieb Anthony Liguori:
>    
>> The use of protocols in backing_files is currently broken because of some
>> checks for adjusting relative pathnames.
>>
>> Additionally, there's a spurious read when using an nbd protocol that can be
>> quite destructive when using copy-on-read.  Potentially, this can lead to
>> probing an image file over top of NBD but this is completely wrong as NBD
>> devices are not growable.
>>
>> Signed-off-by: Anthony Liguori<aliguori@us.ibm.com>
>> ---
>> NB: this is absolutely not ideal.  A more elegant suggestion would be
>> appreciated.  I don't think NBD cleanly fits the model of a protocol as it
>> stands today.
>>
>> diff --git a/block.c b/block.c
>> index cd2ee31..a32d5dd 100644
>> --- a/block.c
>> +++ b/block.c
>> @@ -344,6 +344,12 @@ static int find_image_format(const char *filename, BlockDriver **pdrv)
>>           return ret;
>>       }
>>
>> +    if (strcmp(bs->drv->protocol_name, "nbd") == 0) {
>> +        drv = bs->drv;
>> +        bdrv_delete(bs);
>> +        goto out;
>> +    }
>>      
> Is nbd really the only protocol that behaves like this? I don't like
> hardcoding driver names in generic block layer code.
>    

I'll drop this chunk from the patch as using backing_fmt achieves the 
same goal.  The important hunk is the next one that removes assumptions 
that URIs are filenames.

Regards,

Anthony Liguori

> Kevin
>
Kevin Wolf - Sept. 16, 2010, 2:08 p.m.
Am 16.09.2010 15:00, schrieb Anthony Liguori:
> On 09/16/2010 03:08 AM, Kevin Wolf wrote:
>> Am 11.09.2010 16:04, schrieb Anthony Liguori:
>>    
>>> The use of protocols in backing_files is currently broken because of some
>>> checks for adjusting relative pathnames.
>>>
>>> Additionally, there's a spurious read when using an nbd protocol that can be
>>> quite destructive when using copy-on-read.  Potentially, this can lead to
>>> probing an image file over top of NBD but this is completely wrong as NBD
>>> devices are not growable.
>>>
>>> Signed-off-by: Anthony Liguori<aliguori@us.ibm.com>
>>> ---
>>> NB: this is absolutely not ideal.  A more elegant suggestion would be
>>> appreciated.  I don't think NBD cleanly fits the model of a protocol as it
>>> stands today.
>>>
>>> diff --git a/block.c b/block.c
>>> index cd2ee31..a32d5dd 100644
>>> --- a/block.c
>>> +++ b/block.c
>>> @@ -344,6 +344,12 @@ static int find_image_format(const char *filename, BlockDriver **pdrv)
>>>           return ret;
>>>       }
>>>
>>> +    if (strcmp(bs->drv->protocol_name, "nbd") == 0) {
>>> +        drv = bs->drv;
>>> +        bdrv_delete(bs);
>>> +        goto out;
>>> +    }
>>>      
>> Is nbd really the only protocol that behaves like this? I don't like
>> hardcoding driver names in generic block layer code.
>>    
> 
> I'll drop this chunk from the patch as using backing_fmt achieves the 
> same goal.  The important hunk is the next one that removes assumptions 
> that URIs are filenames.

Right, but next hunk is broken as Juan already mentioned. You always get
a protocol back, so with this patch applied you never resolve relative
paths for backing files.

Kevin
Anthony Liguori - Sept. 16, 2010, 3:40 p.m.
On 09/15/2010 11:06 AM, Juan Quintela wrote:
> Anthony Liguori<aliguori@us.ibm.com>  wrote:
>    
>> The use of protocols in backing_files is currently broken because of some
>> checks for adjusting relative pathnames.
>>
>> Additionally, there's a spurious read when using an nbd protocol that can be
>> quite destructive when using copy-on-read.  Potentially, this can lead to
>> probing an image file over top of NBD but this is completely wrong as NBD
>> devices are not growable.
>>
>> Signed-off-by: Anthony Liguori<aliguori@us.ibm.com>
>> ---
>> NB: this is absolutely not ideal.  A more elegant suggestion would be
>> appreciated.  I don't think NBD cleanly fits the model of a protocol as it
>> stands today.
>>      
> Bad, bad boy, you fixed two things in a single patch.
>    

You're not supposed to notice that :-)  It was an RFC series, I was 
really just looking to start a conversation.

>> @@ -603,10 +610,16 @@ int bdrv_open(BlockDriverState *bs, const char *filename, int flags,
>>           BlockDriver *back_drv = NULL;
>>
>>           bs->backing_hd = bdrv_new("");
>> -        path_combine(backing_filename, sizeof(backing_filename),
>> -                     filename, bs->backing_file);
>> -        if (bs->backing_format[0] != '\0')
>> -            back_drv = bdrv_find_format(bs->backing_format);
>> +        back_drv = bdrv_find_protocol(bs->backing_file);
>> +        if (!back_drv) {
>> +            path_combine(backing_filename, sizeof(backing_filename),
>> +                         filename, bs->backing_file);
>> +            if (bs->backing_format[0] != '\0')
>> +                back_drv = bdrv_find_format(bs->backing_format);
>> +        } else {
>> +            pstrcpy(backing_filename, sizeof(backing_filename),
>> +                    bs->backing_file);
>> +        }
>>
>>           /* backing files always opened read-only */
>>           back_flags =
>>      
> But this one breaks my setup, I have to backout this patch to be able to
> launch guests with qcow2 file images.
>    

Kevin, do you have an opinion about the best way to resolve this?

The relative/absolute path is broken for non-file URIs.  I think we 
could mark a protocol as having a file URI type or we could push the 
absolute/relative conversion down to the file/phys_dev protocol.

I think the former approach is probably the least invasive.

Regards,

Anthony LIguori

> Later, Juan.
>
>
Kevin Wolf - Sept. 17, 2010, 8:53 a.m.
Am 16.09.2010 17:40, schrieb Anthony Liguori:
> On 09/15/2010 11:06 AM, Juan Quintela wrote:
>> Anthony Liguori<aliguori@us.ibm.com>  wrote:
>>    
>>> The use of protocols in backing_files is currently broken because of some
>>> checks for adjusting relative pathnames.
>>>
>>> Additionally, there's a spurious read when using an nbd protocol that can be
>>> quite destructive when using copy-on-read.  Potentially, this can lead to
>>> probing an image file over top of NBD but this is completely wrong as NBD
>>> devices are not growable.
>>>
>>> Signed-off-by: Anthony Liguori<aliguori@us.ibm.com>
>>> ---
>>> NB: this is absolutely not ideal.  A more elegant suggestion would be
>>> appreciated.  I don't think NBD cleanly fits the model of a protocol as it
>>> stands today.
>>>      
>> Bad, bad boy, you fixed two things in a single patch.
>>    
> 
> You're not supposed to notice that :-)  It was an RFC series, I was 
> really just looking to start a conversation.
> 
>>> @@ -603,10 +610,16 @@ int bdrv_open(BlockDriverState *bs, const char *filename, int flags,
>>>           BlockDriver *back_drv = NULL;
>>>
>>>           bs->backing_hd = bdrv_new("");
>>> -        path_combine(backing_filename, sizeof(backing_filename),
>>> -                     filename, bs->backing_file);
>>> -        if (bs->backing_format[0] != '\0')
>>> -            back_drv = bdrv_find_format(bs->backing_format);
>>> +        back_drv = bdrv_find_protocol(bs->backing_file);
>>> +        if (!back_drv) {
>>> +            path_combine(backing_filename, sizeof(backing_filename),
>>> +                         filename, bs->backing_file);
>>> +            if (bs->backing_format[0] != '\0')
>>> +                back_drv = bdrv_find_format(bs->backing_format);
>>> +        } else {
>>> +            pstrcpy(backing_filename, sizeof(backing_filename),
>>> +                    bs->backing_file);
>>> +        }
>>>
>>>           /* backing files always opened read-only */
>>>           back_flags =
>>>      
>> But this one breaks my setup, I have to backout this patch to be able to
>> launch guests with qcow2 file images.
>>    
> 
> Kevin, do you have an opinion about the best way to resolve this?
> 
> The relative/absolute path is broken for non-file URIs.  I think we 
> could mark a protocol as having a file URI type or we could push the 
> absolute/relative conversion down to the file/phys_dev protocol.
> 
> I think the former approach is probably the least invasive.

I agree. It gets a bit hard for things like blkdebug or blkverify which
actually get two file names, but I guess they usually won't be used as a
backing file - and if they do, it's probably reasonable to require
absolute paths or to resolve them relative to the working directory
instead of the image. They are only for debugging anyway.

Kevin

Patch

diff --git a/block.c b/block.c
index cd2ee31..a32d5dd 100644
--- a/block.c
+++ b/block.c
@@ -344,6 +344,12 @@  static int find_image_format(const char *filename, BlockDriver **pdrv)
         return ret;
     }
 
+    if (strcmp(bs->drv->protocol_name, "nbd") == 0) {
+        drv = bs->drv;
+        bdrv_delete(bs);
+        goto out;
+    }
+
     /* Return the raw BlockDriver * to scsi-generic devices or empty drives */
     if (bs->sg || !bdrv_is_inserted(bs)) {
         bdrv_delete(bs);
@@ -373,6 +379,7 @@  static int find_image_format(const char *filename, BlockDriver **pdrv)
             }
         }
     }
+out:
     if (!drv) {
         ret = -ENOENT;
     }
@@ -603,10 +610,16 @@  int bdrv_open(BlockDriverState *bs, const char *filename, int flags,
         BlockDriver *back_drv = NULL;
 
         bs->backing_hd = bdrv_new("");
-        path_combine(backing_filename, sizeof(backing_filename),
-                     filename, bs->backing_file);
-        if (bs->backing_format[0] != '\0')
-            back_drv = bdrv_find_format(bs->backing_format);
+        back_drv = bdrv_find_protocol(bs->backing_file);
+        if (!back_drv) {
+            path_combine(backing_filename, sizeof(backing_filename),
+                         filename, bs->backing_file);
+            if (bs->backing_format[0] != '\0')
+                back_drv = bdrv_find_format(bs->backing_format);
+        } else {
+            pstrcpy(backing_filename, sizeof(backing_filename),
+                    bs->backing_file);
+        }
 
         /* backing files always opened read-only */
         back_flags =