Re: [PATCH 1/2] allow md_flush_request to take NULL bio

[NeilBrown <nfbrown@novell.com>]

[-- Attachment #1: Type: text/plain, Size: 1333 bytes --]

On Thu, Feb 11 2016, Song Liu wrote:

> Signed-off-by: Song Liu <songliubraving@fb.com>
> Signed-off-by: Shaohua Li <shli@fb.com>
> ---
>  drivers/md/md.c | 3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/drivers/md/md.c b/drivers/md/md.c
> index e55e6cf..2997104 100644
> --- a/drivers/md/md.c
> +++ b/drivers/md/md.c
> @@ -406,6 +406,8 @@ static void md_submit_flush_data(struct work_struct *ws)
>  	struct mddev *mddev = container_of(ws, struct mddev, flush_work);
>  	struct bio *bio = mddev->flush_bio;
>  
> +	if (!bio)
> +		goto out;
>  	if (bio->bi_iter.bi_size == 0)
>  		/* an empty barrier - all done */
>  		bio_endio(bio);
> @@ -415,6 +417,7 @@ static void md_submit_flush_data(struct work_struct *ws)
>  	}
>  
>  	mddev->flush_bio = NULL;
> +out:
>  	wake_up(&mddev->sb_wait);
>  }
>  

I don't think this is safe.
->flush_bio is used for two different purposes.
One is to carry the bio so it can be passed to ->make_request after the
flush.
The other is to ensure exclusive access to mddev->flush_work.
The first thing md_flush_request() does is wait for ->flush_bio to be
NULL.  Then it knows that ->flush_work is no longer in use.
With your change you could get md_flush_request trying to INIT_WORK
->flush_work while it is already queued.

Not good.

NeilBrown

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 818 bytes --]