Re: [Xen-devel] [PATCH 3/3] xen-blkback: fix shutdown race

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 28.01.14 at 18:43, Roger Pau Monne <roger.pau@xxxxxxxxxx> 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


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel