Re: [Qemu-block] [Qemu-devel] [PATCH] raw-posix: add 'offset' and 'size' options

[Tomáš Golembiovský]

On Mon, 3 Oct 2016 11:52:59 +0100
"Daniel P. Berrange" <address@hidden> wrote:

> On Mon, Oct 03, 2016 at 12:45:57PM +0200, Tomáš Golembiovský wrote:
> > On Mon, 3 Oct 2016 09:52:13 +0100
> > "Daniel P. Berrange" <address@hidden> wrote:
> >   
> > > On Sun, Oct 02, 2016 at 09:13:29PM +0200, Tomáš Golembiovský wrote:  
> > > > Added two new options 'offset' and 'size'. This makes it possible to use
> > > > only part of the file as a device. This can be used e.g. to limit the
> > > > access only to single partition in a disk image or use a disk inside a
> > > > tar archive (like OVA).
> > > > 
> > > > For now this is only possible for files in read-only mode. It should be
> > > > possible to extend it later to allow read-write mode, but would probably
> > > > require that the size of the device is kept constant (i.e. no resizing).
> > > > 
> > > > Signed-off-by: Tomáš Golembiovský <address@hidden>
> > > > ---
> > > >  block/raw-posix.c | 97 
> > > > +++++++++++++++++++++++++++++++++++++++++++++++++++----
> > > >  1 file changed, 91 insertions(+), 6 deletions(-)    
> > > 
> > > An equivalent change is needed to raw-win32.c  
> > 
> > That's what I feared.
> > 
> >   
> > > > 
> > > > diff --git a/block/raw-posix.c b/block/raw-posix.c
> > > > index 6ed7547..36f2712 100644
> > > > --- a/block/raw-posix.c
> > > > +++ b/block/raw-posix.c    
> > >   
> > > > @@ -421,6 +435,17 @@ static int raw_open_common(BlockDriverState *bs, 
> > > > QDict *options,
> > > >          goto fail;
> > > >      }
> > > >  
> > > > +    s->offset = qemu_opt_get_size(opts, "offset", 0);
> > > > +    s->assumed_size = qemu_opt_get_size(opts, BLOCK_OPT_SIZE, 0);    
> > > 
> > > If the user didn't provide "size", then we should initialize
> > > it to the actual size of the underlying storage, rather than
> > > to 0.  
> > 
> > I rather wanted to distinguish  the case when no offset or size was
> > specified to be able to limit the scope of introduced changes.  
> 
> IMHO that's a mistake - it leads to 2 different codepaths one of
> which will be mostly unused & untested, so liable to bitrot.

Hmm, valid point.


> 
> > > > +
> > > > +    if (((bs->drv != &bdrv_file) || !bs->read_only) &&    
> > > 
> > > Why the check against bdrv_file ?  
> > 
> > To limit it only to files. Maybe there is better way to do that? The
> > devices have a nasty habit to change the size. Sure, this can happen to
> > file too, e.g. if somebody truncates the file outside QEMU. But that's
> > rather a bad behaviour. For devices changing the size may be perfectly
> > valid operation, e.g. replacing CD in drive or card in a card reader.  
> 
> The raw driver is usable over any storage backend (file, rbd, iscsi,
> etc, etc) and it is valid to want to use a offset/size parameter in
> combination with any of them. So we should not restrict it to just
> files.

The question is then, what to do when the underlying device changes? The
size/offset may not be valid at that point anymore.


> > > > +        ((s->offset > 0) || (s->assumed_size > 0))) {
> > > > +        error_setg(errp, "offset and size options are allowed only for 
> > > > "
> > > > +                         "files in read-only mode");
> > > > +        ret = -EINVAL;
> > > > +        goto fail;
> > > > +    }    
> > > 
> > > Why did you restrict it to read-only instead of adding the offset logic
> > > to the write & truncate methods. IMHO if we're going to add this feature
> > > we should make it work in all scenarios, not just do 1/2 the job.  
> > 
> > I still plan to do that, but I didn't feel confident enough to do it in
> > the first run.
> > 
> > We need to decide what is the correct behaviour here first. Since the
> > image we're working with is contained in some larger unit it cannot be
> > resized freely. There are two option that come to my mind:
> > 
> > 1) block truncate/grow completely,
> > 2) allow image to be truncated and grown only if the new size does not
> >    go over the original size.
> > 
> > If we go with the second option then I have a another question. How
> > strict is (or should be) qemu about the sizes being block aligned? I'm
> > still little bit fuzzy on that. Somewhere it is assumed that the size is
> > multiple of 512, sometimes qemu automatically rounds up if it isn't and
> > sometimes it seems to me that the size can be arbitrary.  
> 
> We should not make assumptions about what is a valid or invalid usage,
> as QEMU doesn't have enough knowledge to do that correctly.
> 
> IOW, we should just allow truncate with no restrictions. It is up to the
> calling app to figure out whether truncate makes sense or not.

Maybe I'm missing something here. Can the truncate operation be somehow
initiated from inside the VM, or is that only something that can be done
from outside with 'qemu-img resize'?


    Tomas

-- 
Tomáš Golembiovský <address@hidden>