Re: [PATCH 1/7] block: Add block_flush_device()

Linus Torvalds
2009-03-31T17:11:51Z

On Tue, 31 Mar 2009, Jens Axboe wrote:
>
> So here's a test patch that attempts to just ignore such a failure to
> flush the caches.

I suspect you should not do it like this.

> diff --git a/fs/bio.c b/fs/bio.c
> index a040cde..79e3cec 100644
> --- a/fs/bio.c
> +++ b/fs/bio.c
> @@ -1380,7 +1380,17 @@ void bio_check_pages_dirty(struct bio *bio)
> **/
> void bio_endio(struct bio *bio, int error)
> {
> - if (error)
> + /*
> + * Special case here - hide the -EOPNOTSUPP from the driver or
> + * block layer, dump a warning the first time this happens so that
> + * the admin knows that we may not provide the ordering guarantees
> + * that are needed. Don't clear the uptodate bit.
> + */
> + if (error == -EOPNOTSUPP && bio_barrier(bio)) {
> + set_bit(BIO_EOPNOTSUPP, &bio->bi_flags);
> + blk_queue_set_noflush(bio->bi_bdev);
> + error = 0;
> + } else if (error)

I suspect this part is just wrong.

I could easily imagine a driver that returns EOPNOTSUPP only for a certain
_kind_ of bio.

For example, if the drive doesn't support FUA, then you cannot do a
serialized IO operation, but you can still mostly do a serialized op
without any IO attached to it.

IOW, the "empty flush" really _is_ special. An this check should not be in
the generic "bio_endio()" case, it should only be in the special
blkdev_issue_flush() case.

I think. No?

Linus