| Message ID | 1284213896-12705-3-git-send-email-aliguori@us.ibm.com |
|---|---|
| State | New |
| Headers | show |
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
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 > >
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 >> >
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.
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
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 >
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
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. > >
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
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 =
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.