[PATCH] block: silently error unsupported empty barriers too

[PATCH] block: silently error unsupported empty barriers too

[Mark McLoughlin]

With 2.6.31-rc5 in a KVM guest using dm and virtio_blk, we see the
following errors:

  end_request: I/O error, dev vda, sector 0
  end_request: I/O error, dev vda, sector 0

The errors go away if dm stops submitting empty barriers, by reverting:

  commit 52b1fd5a27c625c78373e024bf570af3c9d44a79
  Author: Mikulas Patocka <mpatocka@redhat.com>
    dm: send empty barriers to targets in dm_flush

We should error all barriers, even empty barriers, on devices like
virtio_blk which don't support them.

See also:

  https://bugzilla.redhat.com/514901

Signed-off-by: Mark McLoughlin <markmc@redhat.com>
Cc: Rusty Russell <rusty@rustcorp.com.au>
Cc: Mikulas Patocka <mpatocka@redhat.com>
Cc: Alasdair G Kergon <agk@redhat.com>
Cc: Neil Brown <neilb@suse.de>
---
 block/blk-core.c |    3 +--
 1 files changed, 1 insertions(+), 2 deletions(-)

diff --git a/block/blk-core.c b/block/blk-core.c
index e3299a7..35ad2bb 100644
--- a/block/blk-core.c
+++ b/block/blk-core.c
@@ -1163,8 +1163,7 @@ static int __make_request(struct request_queue *q, struct bio *bio)
 	const int unplug = bio_unplug(bio);
 	int rw_flags;
 
-	if (bio_barrier(bio) && bio_has_data(bio) &&
-	    (q->next_ordered == QUEUE_ORDERED_NONE)) {
+	if (bio_barrier(bio) && (q->next_ordered == QUEUE_ORDERED_NONE)) {
 		bio_endio(bio, -EOPNOTSUPP);
 		return 0;
 	}
-- 
1.6.4

Re: [dm-devel] [PATCH] block: silently error unsupported empty barriers too

[Alasdair G Kergon]

On Thu, Aug 06, 2009 at 12:14:17PM +0100, Mark McLoughlin wrote:
> We should error all barriers, even empty barriers, on devices like
> virtio_blk which don't support them.
 
Have you considered whether or not virtio_blk actually needs to
support empty barriers?

Alasdair

Re: [dm-devel] [PATCH] block: silently error unsupported empty barriers too

[Christoph Hellwig]

On Thu, Aug 06, 2009 at 12:45:50PM +0100, Alasdair G Kergon wrote:
> On Thu, Aug 06, 2009 at 12:14:17PM +0100, Mark McLoughlin wrote:
> > We should error all barriers, even empty barriers, on devices like
> > virtio_blk which don't support them.
>  
> Have you considered whether or not virtio_blk actually needs to
> support empty barriers?

virtio_blk on kvm does not support any barriers at all, similar to many
other drivers out there.  If the queue flags say we don't support
barriers higher layers should not submit any barrier requests.

Re: [dm-devel] [PATCH] block: silently error unsupported empty barriers too

[Mikulas Patocka]

On Thu, 6 Aug 2009, Alasdair G Kergon wrote:

> On Thu, Aug 06, 2009 at 12:14:17PM +0100, Mark McLoughlin wrote:
> > We should error all barriers, even empty barriers, on devices like
> > virtio_blk which don't support them.
>  
> Have you considered whether or not virtio_blk actually needs to
> support empty barriers?
> 
> Alasdair

This is only for request-based drivers, where it is the responsibility of 
blk-core to translate barriers. I think the empty barrier request anyway 
in blk_do_ordered, but with an error message. So the patch changes it to 
discard it early and queitly. It seems ok.

Mikulas

Re: [dm-devel] [PATCH] block: silently error unsupported empty barriers too

[Andrew Patterson]

On Thu, 2009-08-06 at 21:50 -0400, Mikulas Patocka wrote:
> On Thu, 6 Aug 2009, Alasdair G Kergon wrote:
> 
> > On Thu, Aug 06, 2009 at 12:14:17PM +0100, Mark McLoughlin wrote:
> > > We should error all barriers, even empty barriers, on devices like
> > > virtio_blk which don't support them.
> >  
> > Have you considered whether or not virtio_blk actually needs to
> > support empty barriers?
> > 
> > Alasdair
> 
> This is only for request-based drivers, where it is the responsibility of 
> blk-core to translate barriers. I think the empty barrier request anyway 
> in blk_do_ordered, but with an error message. So the patch changes it to 
> discard it early and queitly. It seems ok.
> 
> Mikulas
> --

This patch seems to have been dropped.  I can reproduce the error using
the following:

     1. Create an LVM logical volume on top of a cciss device (note that
        the cciss driver does not support barriers)
     2. Create an ext3 file system on top of the logical volume
     3. Mount the file-system using -obarrier=1
     4. Copy some files onto the file-system
     5. Run sync (dm_flush is called)

If I apply the patch (with some munging), the 

"end_request: I/O error, dev cciss/cXdY, sector 0"

messages go away.

This is not strictly a regression, given that the problem was introduced
in 2.6.31, but I think it should still be fixed for 2.6.32.

Andrew

-- 
Andrew Patterson
Hewlett-Packard

Re: block: silently error unsupported empty barriers too

[Mike Snitzer]

On Wed, Oct 28 2009 at  4:19pm -0400,
Andrew Patterson <andrew.patterson@hp.com> wrote:

> On Thu, 2009-08-06 at 21:50 -0400, Mikulas Patocka wrote:
> > On Thu, 6 Aug 2009, Alasdair G Kergon wrote:
> > 
> > > On Thu, Aug 06, 2009 at 12:14:17PM +0100, Mark McLoughlin wrote:
> > > > We should error all barriers, even empty barriers, on devices like
> > > > virtio_blk which don't support them.
> > >  
> > > Have you considered whether or not virtio_blk actually needs to
> > > support empty barriers?
> > > 
> > > Alasdair
> > 
> > This is only for request-based drivers, where it is the responsibility of 
> > blk-core to translate barriers. I think the empty barrier request anyway 
> > in blk_do_ordered, but with an error message. So the patch changes it to 
> > discard it early and queitly. It seems ok.
> > 
> > Mikulas
> > --
> 
> This patch seems to have been dropped.  I can reproduce the error using
> the following:
> 
>      1. Create an LVM logical volume on top of a cciss device (note that
>         the cciss driver does not support barriers)
>      2. Create an ext3 file system on top of the logical volume
>      3. Mount the file-system using -obarrier=1
>      4. Copy some files onto the file-system
>      5. Run sync (dm_flush is called)
> 
> If I apply the patch (with some munging), the 
> 
> "end_request: I/O error, dev cciss/cXdY, sector 0"
> 
> messages go away.
> 
> This is not strictly a regression, given that the problem was introduced
> in 2.6.31, but I think it should still be fixed for 2.6.32.

It was resubmitted last Friday:
http://lkml.org/lkml/2009/10/23/196

And is queued in Jens' 'for-linus' branch (meaning it should get
upstream in time for 2.6.32), see:
http://git.kernel.org/?p=linux/kernel/git/axboe/linux-2.6-block.git;a=commit;h=6cafb12dc85a5bdc722791cc5070968413264909

Mike

Re: block: silently error unsupported empty barriers too

[Jens Axboe]

On Wed, Oct 28 2009, Mike Snitzer wrote:
> On Wed, Oct 28 2009 at  4:19pm -0400,
> Andrew Patterson <andrew.patterson@hp.com> wrote:
> 
> > On Thu, 2009-08-06 at 21:50 -0400, Mikulas Patocka wrote:
> > > On Thu, 6 Aug 2009, Alasdair G Kergon wrote:
> > > 
> > > > On Thu, Aug 06, 2009 at 12:14:17PM +0100, Mark McLoughlin wrote:
> > > > > We should error all barriers, even empty barriers, on devices like
> > > > > virtio_blk which don't support them.
> > > >  
> > > > Have you considered whether or not virtio_blk actually needs to
> > > > support empty barriers?
> > > > 
> > > > Alasdair
> > > 
> > > This is only for request-based drivers, where it is the responsibility of 
> > > blk-core to translate barriers. I think the empty barrier request anyway 
> > > in blk_do_ordered, but with an error message. So the patch changes it to 
> > > discard it early and queitly. It seems ok.
> > > 
> > > Mikulas
> > > --
> > 
> > This patch seems to have been dropped.  I can reproduce the error using
> > the following:
> > 
> >      1. Create an LVM logical volume on top of a cciss device (note that
> >         the cciss driver does not support barriers)
> >      2. Create an ext3 file system on top of the logical volume
> >      3. Mount the file-system using -obarrier=1
> >      4. Copy some files onto the file-system
> >      5. Run sync (dm_flush is called)
> > 
> > If I apply the patch (with some munging), the 
> > 
> > "end_request: I/O error, dev cciss/cXdY, sector 0"
> > 
> > messages go away.
> > 
> > This is not strictly a regression, given that the problem was introduced
> > in 2.6.31, but I think it should still be fixed for 2.6.32.
> 
> It was resubmitted last Friday:
> http://lkml.org/lkml/2009/10/23/196
> 
> And is queued in Jens' 'for-linus' branch (meaning it should get
> upstream in time for 2.6.32), see:
> http://git.kernel.org/?p=linux/kernel/git/axboe/linux-2.6-block.git;a=commit;h=6cafb12dc85a5bdc722791cc5070968413264909

I sent out a pull request yesterday, but it isn't in yet. Any day now...

-- 
Jens Axboe