Re: [PATCH v2 for-5.0] xen-block: Fix double qlist remove and request leak
On 06.04.20 16:02, Anthony PERARD wrote:
> Commit a31ca6801c02 ("qemu/queue.h: clear linked list pointers on
> remove") revealed that a request was removed twice from a list, once
> in xen_block_finish_request() and a second time in
> xen_block_release_request() when both function are called from
> xen_block_complete_aio(). But also, the `requests_inflight' counter is
> decreased twice, and thus became negative.
>
> This is a bug that was introduced in bfd0d6366043, where a `finished'
> list was removed.
>
> That commit also introduced a leak of request in xen_block_do_aio().
> That function calls xen_block_finish_request() but the request is
> never released after that.
>
> To fix both issue, we do two changes:
> - we squash finish_request() and release_request() together as we want
> to remove a request from 'inflight' list to add it to 'freelist'.
> - before releasing a request, we need to let now the result to the
> other end, thus we should call xen_block_send_response() before
> releasing a request.
>
> The first change fix the double QLIST_REMOVE() as we remove the extra
> call. The second change makes the leak go away because if we want to
> call finish_request(), we need to call a function that do all of
> finish, send response, and release.
>
> Fixes: bfd0d6366043 ("xen-block: improve response latency")
> Signed-off-by: Anthony PERARD
> ---
> hw/block/dataplane/xen-block.c | 48 --
> 1 file changed, 16 insertions(+), 32 deletions(-)
I’m going to send a pull request today anyway, so I hope you won’t mind
and let me take this patch to my branch (with Paul’s suggestions
incorporated):
https://git.xanclic.moe/XanClic/qemu/commits/branch/block
Max
signature.asc
Description: OpenPGP digital signature
Re: [Xen-devel] [PATCH] xen-block: support feature-large-sector-size
On 26.06.19 19:19, Anthony PERARD wrote: > On Wed, Jun 26, 2019 at 06:48:50PM +0200, Max Reitz wrote: >> On 09.04.19 18:40, Paul Durrant wrote: >>> A recent Xen commit [1] clarified the semantics of sector based quantities >>> used in the blkif protocol such that it is now safe to create a xen-block >>> device with a logical_block_size != 512, as long as the device only >>> connects to a frontend advertizing 'feature-large-block-size'. >>> >>> This patch modifies xen-block accordingly. It also uses a stack variable >>> for the BlockBackend in xen_block_realize() to avoid repeated dereferencing >>> of the BlockConf pointer, and changes the parameters of >>> xen_block_dataplane_create() so that the BlockBackend pointer and sector >>> size are passed expicitly rather than implicitly via the BlockConf. >>> >>> These modifications have been tested against a recent Windows PV XENVBD >>> driver [2] using a xen-disk device with a 4kB logical block size. >>> >>> [1] >>> http://xenbits.xen.org/gitweb/?p=xen.git;a=commit;h=67e1c050e36b2c9900cca83618e56189effbad98 >>> [2] https://winpvdrvbuild.xenproject.org:8080/job/XENVBD-master/126 >>> >>> Signed-off-by: Paul Durrant >>> --- >>> Cc: Stefano Stabellini >>> Cc: Anthony Perard >>> Cc: Stefan Hajnoczi >>> Cc: Kevin Wolf >>> Cc: Max Reitz >>> --- >>> hw/block/dataplane/xen-block.c | 25 -- >>> hw/block/dataplane/xen-block.h | 3 ++- >>> hw/block/xen-block.c | 38 +- >>> 3 files changed, 40 insertions(+), 26 deletions(-) >> >> Thanks, added “by frontend” to the error message and applied to my block >> branch: >> >> https://git.xanclic.moe/XanClic/qemu/commits/branch/block > > :(, I've just sent a pull request with that patch: > https://patchew.org/QEMU/20190624153257.20163-1-anthony.per...@citrix.com/20190624153257.20163-2-anthony.per...@citrix.com/ That’s just as well, then. :-) > I guess I need to start sending an email every time I've added a patch > to my queue. Well, it certainly won’t hurt. Although in this cases it’s just a bit of an unfortunate coincidence that I looked at this patch now when Peter seems to be away (otherwise I’d have seen it in master). Max signature.asc Description: OpenPGP digital signature ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH] xen-block: support feature-large-sector-size
On 09.04.19 18:40, Paul Durrant wrote: > A recent Xen commit [1] clarified the semantics of sector based quantities > used in the blkif protocol such that it is now safe to create a xen-block > device with a logical_block_size != 512, as long as the device only > connects to a frontend advertizing 'feature-large-block-size'. > > This patch modifies xen-block accordingly. It also uses a stack variable > for the BlockBackend in xen_block_realize() to avoid repeated dereferencing > of the BlockConf pointer, and changes the parameters of > xen_block_dataplane_create() so that the BlockBackend pointer and sector > size are passed expicitly rather than implicitly via the BlockConf. > > These modifications have been tested against a recent Windows PV XENVBD > driver [2] using a xen-disk device with a 4kB logical block size. > > [1] > http://xenbits.xen.org/gitweb/?p=xen.git;a=commit;h=67e1c050e36b2c9900cca83618e56189effbad98 > [2] https://winpvdrvbuild.xenproject.org:8080/job/XENVBD-master/126 > > Signed-off-by: Paul Durrant > --- > Cc: Stefano Stabellini > Cc: Anthony Perard > Cc: Stefan Hajnoczi > Cc: Kevin Wolf > Cc: Max Reitz > --- > hw/block/dataplane/xen-block.c | 25 -- > hw/block/dataplane/xen-block.h | 3 ++- > hw/block/xen-block.c | 38 +- > 3 files changed, 40 insertions(+), 26 deletions(-) Thanks, added “by frontend” to the error message and applied to my block branch: https://git.xanclic.moe/XanClic/qemu/commits/branch/block Max signature.asc Description: OpenPGP digital signature ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH] qemu: include generated files with <> and not ""
On 2018-03-20 14:41, Daniel P. Berrangé wrote: > On Tue, Mar 20, 2018 at 02:32:16PM +0100, Gerd Hoffmann wrote: >> Hi, >> So for these, we should use "". None of these are generated files though. >>> >>> That leads to crazy inconsistent message for developers where 50% of QEMU >>> header files must use <> and the other 50% of header files must use "". >> >> The rules are pretty simple though: >> >>(1) Headers which are generated use <>. >>(2) Headers which are in include/ use <>. >>(3) Headers sitting in the same directory as the source files use "". > > We have 1200 header files in QEMU - a developer might just reasonably remember > which header file is a QEMU header, vs which is a 3rd party system header. > Expecting devs to remember which of those 3 buckets each QEMU header falls > under is unreasonable IMHO. I agree with this in principle, but I don't find it unreasonable for someone to look up whether a header file exists in the same directory. I find that much simpler than to look up whether it is a system header, actually. And I have to agree with Michael that his rule when to use "" (if the file is in the same directory) and when to use <> (otherwise) is actually what every C developer might do by instinct. (I guess it's also easier to check in a script than the original guideline?) However, I also think that if any problem arises because "" was used for a generated file and that then uses a stale file, the bug is somewhere else. And I think that while Michael's proposal is the more intuitive (C) way, it is a tiny bit more complicated to explain than 'Use "" for qemu headers, <> for everything else'. But I guess the main advantage with using this rule I see is that it's better for people reading the code. It's just nice to know whether a file belongs to qemu or not by just looking at the #include statement. (Note that this implies that it is indeed more difficult to determine whether a header belongs to qemu than whether it sits in the same directory as the C file, though!) So I think the old (current) rule is better for reading code, Michael's rule would be better for writing code. I think reading code is what should be easier. But since that may be eaten up by build breakages due to stale files, I don't have a strong opinion either way. I just wanted to chime in because in my opinion 'Use "" for headers in the same directory, <> for everything else' is by no means an unreasonably complicated rule for people writing code. Max signature.asc Description: OpenPGP digital signature ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
