lkml.org 
[lkml]   [2014]   [Jan]   [29]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [Xen-devel] [PATCH 3/3] xen-blkback: fix shutdown race
>>> On 28.01.14 at 18:43, Roger Pau Monne <roger.pau@citrix.com> wrote:
> --- a/drivers/block/xen-blkback/blkback.c
> +++ b/drivers/block/xen-blkback/blkback.c
> @@ -985,17 +985,31 @@ static void __end_block_io_op(struct pending_req
> *pending_req, int error)
> * the proper response on the ring.
> */
> if (atomic_dec_and_test(&pending_req->pendcnt)) {
> - xen_blkbk_unmap(pending_req->blkif,
> + struct xen_blkif *blkif = pending_req->blkif;
> +
> + xen_blkbk_unmap(blkif,
> pending_req->segments,
> pending_req->nr_pages);
> - make_response(pending_req->blkif, pending_req->id,
> + make_response(blkif, pending_req->id,
> pending_req->operation, pending_req->status);
> - xen_blkif_put(pending_req->blkif);
> - if (atomic_read(&pending_req->blkif->refcnt) <= 2) {
> - if (atomic_read(&pending_req->blkif->drain))
> - complete(&pending_req->blkif->drain_complete);
> + free_req(blkif, pending_req);
> + /*
> + * Make sure the request is freed before releasing blkif,
> + * or there could be a race between free_req and the
> + * cleanup done in xen_blkif_free during shutdown.
> + *
> + * NB: The fact that we might try to wake up pending_free_wq
> + * before drain_complete (in case there's a drain going on)
> + * it's not a problem with our current implementation
> + * because we can assure there's no thread waiting on
> + * pending_free_wq if there's a drain going on, but it has
> + * to be taken into account if the current model is changed.
> + */
> + xen_blkif_put(blkif);
> + if (atomic_read(&blkif->refcnt) <= 2) {
> + if (atomic_read(&blkif->drain))
> + complete(&blkif->drain_complete);
> }
> - free_req(pending_req->blkif, pending_req);
> }
> }

The put is still too early imo - you're explicitly accessing field in the
structure immediately afterwards. This may not be an issue at
present, but I think it's at least a latent one.

Apart from that, the two if()s would - at least to me - be more
clear if combined into one.

Jan



\
 
 \ /
  Last update: 2014-01-29 10:21    [W:0.082 / U:1.252 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site