[BUG] + possible [PATCH]. blkdev_issue_flush is a NOOP for request based drivers

[BUG] + possible [PATCH]. blkdev_issue_flush is a NOOP for request based drivers

[Alex Bligh]

blkdev_issue_flush appears to be a NOOP for request-based srivers, because
it returns -ENXIO if no make_request_function is present. AIUI only bio
based drivers have a make_request_function. The preceding comment suggests
this is an attempt to avoid making a request to bio based drivers before
their queue is set up. This should not exclude request based drivers from
getting any flushes. Instead therefore check for EITHER
make_request_function OR request_function and queue flags. The check for
queue flags may be unnecessary as the block layer may simply not send the
flush on.

-- 
Alex Bligh

Signed-Off-By: Alex Bligh <alex@alex.org.uk>

diff --git a/block/blk-flush.c b/block/blk-flush.c
index 6c9b5e1..3a6d4bd 100644
--- a/block/blk-flush.c
+++ b/block/blk-flush.c
@@ -405,13 +405,14 @@ int blkdev_issue_flush(struct block_device *bdev, 
gfp_t gfp_mask,
        /*
         * some block devices may not have their queue correctly set up here
         * (e.g. loop device without a backing file) and so issuing a flush
         * here will panic. Ensure there is a request function before 
issuing
         * the flush.
         */
-       if (!q->make_request_fn)
+       if (!q->make_request_fn &&
+           !(q->request_fn && (q->flush_flags & REQ_FLUSH)))
                return -ENXIO;

        bio = bio_alloc(gfp_mask, 0);
        bio->bi_end_io = bio_end_flush;
        bio->bi_bdev = bdev;
        bio->bi_private = &wait;

Re: [BUG] + possible [PATCH]. blkdev_issue_flush is a NOOP for request based drivers

[Tejun Heo]

Hello, Alex.

I'm confused (as usual).

On Thu, May 26, 2011 at 12:00:36PM +0100, Alex Bligh wrote:
> blkdev_issue_flush appears to be a NOOP for request-based srivers, because

Ummm... request based drivers have __make_request as its
->make_request_fn() which is set during queue allocation.

> it returns -ENXIO if no make_request_function is present. AIUI only bio
> based drivers have a make_request_function. The preceding comment suggests
> this is an attempt to avoid making a request to bio based drivers before
> their queue is set up.

If you look at the original commit which added the condition,

commit f10d9f617a65905c556c3b37c9b9646ae7d04ed7
Author: Dave Chinner <dchinner@redhat.com>
Date:   Tue Jul 13 17:50:50 2010 +1000

    blkdev: check for valid request queue before issuing flush

    Issuing a blkdev_issue_flush() on an unconfigured loop device causes a panic as
    q->make_request_fn is not configured. This can occur when trying to mount the
    unconfigured loop device as an XFS filesystem. There are no guards that catch
    the bio before the request function is called because we don't add a payload to
    the bio. Instead, manually check this case as soon as we have a pointer to the
    queue to flush.

    Signed-off-by: Dave Chinner <dchinner@redhat.com>
    Signed-off-by: Jens Axboe <jaxboe@fusionio.com>
				    
It was added to work around uniitialized queue from loop device.  I
think this is the wrong place to check for this tho.  We should have
made loop to always initialize queue and use placeholder
make_request_fn for inactive devices.

> This should not exclude request based drivers from
> getting any flushes. Instead therefore check for EITHER
> make_request_function OR request_function and queue flags. The check for
> queue flags may be unnecessary as the block layer may simply not send the
> flush on.

Have you tested the code actually works as you expect it to?  Before
and after?

Thanks.

-- 
tejun

Re: [BUG] + possible [PATCH]. blkdev_issue_flush is a NOOP for request based drivers

[Alex Bligh]

--On 26 May 2011 14:53:47 +0200 Tejun Heo <tj@kernel.org> wrote:

>> This should not exclude request based drivers from
>> getting any flushes. Instead therefore check for EITHER
>> make_request_function OR request_function and queue flags. The check for
>> queue flags may be unnecessary as the block layer may simply not send the
>> flush on.
>
> Have you tested the code actually works as you expect it to?  Before
> and after?

Nope - I meant to indicate that. It doesn't break things, but it didn't
get me where I wanted to, but I had put that down to something missing
elsewhere. It may well be unnecessary (as you suggest) in which case
my debugging has taken a wrong turn, and apologies for wasting your time.

-- 
Alex Bligh