[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH 5/5] block: Move request_alignment into BlockLim
From: |
Kevin Wolf |
Subject: |
Re: [Qemu-devel] [PATCH 5/5] block: Move request_alignment into BlockLimit |
Date: |
Tue, 14 Jun 2016 17:30:00 +0200 |
User-agent: |
Mutt/1.5.21 (2010-09-15) |
Am 14.06.2016 um 16:47 hat Eric Blake geschrieben:
> On 06/14/2016 02:05 AM, Kevin Wolf wrote:
>
> >>>> static void raw_refresh_limits(BlockDriverState *bs, Error **errp)
> >>>> {
> >>>> + /* Inherit all limits except for request_alignment */
> >>>> + int request_alignment = bs->bl.request_alignment;
> >>>> +
> >>>> bs->bl = bs->file->bs->bl;
> >>>> + bs->bl.request_alignment = request_alignment;
> >>
> >> Any ideas on how to fix the test, then? Have two blkdebug devices
> >> nested atop one another, since those are the devices where we can
> >> explicitly override alignment?
> >
> > Interesting idea. Maybe that's a good option if it works.
> >
> >> (normally, you'd _expect_ the chain to
> >> inherit the worst-case alignment of all BDS in the chain, so blkdebug is
> >> the way around it).
> >
> > Actually, I think there are two cases.
> >
> > The first one is that you get a request and want to know what to do with
> > it. Here you don't want to inherit the worst-case alignment. You'd
> > rather want to enforce alignment only when it is actually needed. For
> > example, if you have a cache=none backing file with 4k alignment, but a
> > cache=writeback overlay, and you don't actually access the backing file
> > with this request, it would be wasteful to align the request. You also
> > don't really know that a driver will issue a misaligned request (or any
> > request at all) to the lower layer just because it got called with one.
> >
> > The second case is when you want to issue a request. For example, let's
> > take the qcow2 cache here, which has 64k cached and needs to update a
> > few bytes. Currently it always writes the full 64k (and with 1 MB
> > clusters a full megabyte), but what it really should do is consider the
> > worst-case alignment.
> >
> > This is comparable to the difference between bdrv_opt_mem_align(), which
> > is used in qemu_blockalign() where we want to create a buffer that works
> > even in the worst case, and bdrv_min_mem_align(), which is used in
> > bdrv_qiov_is_aligned() in order to determine whether we must align an
> > existing request.
> >
> >> That's the only thing left before I repost the series, so I may just
> >> post the last patch as RFC and play with it a bit more...
> >
> > And in the light of the above, maybe the solution is as easy as changing
> > raw_refresh_limits() to set bs->bl.request_alignment = 1.
>
> Or maybe split the main bdrv_refresh_limits() into two pieces: one that
> merges limits from one BDS into another (the limits that are worth
> merging, and in the correct direction: opt merges to MAX, max merges to
> MIN_NON_ZERO, request_alignment is not merged), the other that calls
> merge(bs, bs->file->bs); then have raw_refresh_limits() also use the
> common merge functionality rather than straight copy. Testing that
> approach now...
So you don't agree with what I said above?
I think that block drivers should only set limits that they require for
themselves. The block layer bdrv_refresh_limits() function can then
merge things where needed; drivers shouldn't be involved there.
Kevin
pgpOnaJn5drlN.pgp
Description: PGP signature
- Re: [Qemu-devel] [PATCH 4/5] block: Switch discard length bounds to byte-based, (continued)
Re: [Qemu-devel] [PATCH 5/5] block: Move request_alignment into BlockLimit, Kevin Wolf, 2016/06/07
[Qemu-devel] [PATCH 3/5] block: Switch transfer length bounds to byte-based, Eric Blake, 2016/06/03
[Qemu-devel] [PATCH 1/5] block: Tighter assertions on bdrv_aligned_preadv(), Eric Blake, 2016/06/03
[Qemu-devel] [PATCH 2/5] block: Honor flags during bdrv_aligned_preadv(), Eric Blake, 2016/06/03