Re: [Qemu-devel] [PATCHv3 RESEND] block: introduce BDRV_O_SEQUENTIAL

[Stefan Hajnoczi]

On Thu, Jun 05, 2014 at 10:13:04AM +0200, Kevin Wolf wrote:
> Am 05.06.2014 um 10:09 hat Peter Lieven geschrieben:
> > On 05.06.2014 09:53, Stefan Hajnoczi wrote:
> > >On Wed, Jun 04, 2014 at 05:31:48PM +0200, Peter Lieven wrote:
> > >>Am 04.06.2014 17:12, schrieb Stefan Hajnoczi:
> > >>>On Fri, May 30, 2014 at 11:40:37PM +0200, Peter Lieven wrote:
> > >>>>diff --git a/block/raw-posix.c b/block/raw-posix.c
> > >>>>index 6586a0c..9768cc4 100644
> > >>>>--- a/block/raw-posix.c
> > >>>>+++ b/block/raw-posix.c
> > >>>>@@ -447,6 +447,13 @@ static int raw_open_common(BlockDriverState *bs, 
> > >>>>QDict *options,
> > >>>>      }
> > >>>>  #endif
> > >>>>+#ifdef POSIX_FADV_SEQUENTIAL
> > >>>>+    if (bs->open_flags & BDRV_O_SEQUENTIAL &&
> > >>>>+        !(bs->open_flags & BDRV_O_NOCACHE)) {
> > >>>>+        posix_fadvise(s->fd, 0, 0, POSIX_FADV_SEQUENTIAL);
> > >>>>+    }
> > >>>>+#endif
> > >>>This is only true if the image format is raw.  If the image format on
> > >>>top of this raw-posix BDS is non-raw then the read pattern may not be
> > >>>sequential.
> > >>You are right, but will the other formats set BDRV_O_SEQUENTIAL?
> > >If the user specifies qemu-img convert -N then it will be set for any
> > >image format.
> > 
> > Of course, but when e.g. qcow2 opens its underlying file, then 
> > BDRV_O_SEQUENTIAL
> > is not passed on, or is it?
> 
> It isn't qcow2 but block.c that opens bs->file, and unless you
> explicitly filter out a flag, bs->file inherits it. (If it didn't do
> that, your patch would have no effect for raw either.)

Yes, exactly.  When a raw image file is opened there are actually two
BlockDriverStates:

  raw_bsd ("drive0")
    file: raw-posix (anonymous)

Since your patch affected the buffer cache counter, we know that the
flag was propagated down to raw-posix (by block.c as Kevin explained).

The qcow2 case looks like this:

  qcow2 ("drive0")
    file: raw-posix (anonymous)

> > >Maybe qemu-img convert can always set BDRV_O_SEQUENTIAL and the have the
> > >raw_bsd.c format propagate it to bs->file while other formats do not.
> > >Then the user doesn't have to specify a command-line option and we don't
> > >set it for non-raw image formats.
> > 
> > This would be an option.
> 
> I agree, though it's not quite clear how raw_bsd would do that. Would
> that involve a bdrv_reopen() for bs->file?

One way is to add a BlockDriver bitmask field for options that get
propagated to its children.  Only raw_bsd will include
BDRV_O_SEQUENTIAL.

Stefan