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

[Tejun Heo <tj@kernel.org>]

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