[dm-devel] [PATCH] dm barrier: A better test for -EOPNOTSUPP.

[Mikulas Patocka]

On Thu, 8 Jul 2010, Mike Snitzer wrote:

> On Wed, Jul 07 2010 at  6:22pm -0400,
> Mikulas Patocka <mpatocka at redhat.com> wrote:
> 
> > Hi
> > 
> > This patch removes the second cache flush if discard isn't supported. 
> > The first flush is hard to bypass, so it's not worth doing it.
> > 
> > Mikulas
> > 
> > ---
> > 
> > Don't do the second flush if the request isn't supported.
> > 
> > If the request fails with -EOPNOTSUPP, don't perform the second flush.
> > This can happen with discard+barrier requests. If the device doesn't support
> > discard, there would be two useless SYNCHRONIZE CACHE commands.
> > 
> > The first dm_flush cannot be so easily optimized out, so we leave it there.
> > 
> > Signed-off-by: Mikulas Patocka <mpatocka at redhat.com>
> > 
> > ---
> >  drivers/md/dm.c |    7 ++++++-
> >  1 file changed, 6 insertions(+), 1 deletion(-)
> > 
> > Index: linux-2.6.35-rc3-fast/drivers/md/dm.c
> > ===================================================================
> > --- linux-2.6.35-rc3-fast.orig/drivers/md/dm.c	2010-07-08 00:11:05.000000000 +0200
> > +++ linux-2.6.35-rc3-fast/drivers/md/dm.c	2010-07-08 00:12:02.000000000 +0200
> > @@ -2365,7 +2365,12 @@ static void process_barrier(struct mappe
> >  
> >  	if (!bio_empty_barrier(bio)) {
> >  		__split_and_process_bio(md, bio);
> > -		dm_flush(md);
> > +		/*
> > +		 * If the request isn't supported, don't waste time with
> > +		 * the second flush.
> > +		 */
> > +		if (md->barrier_error != -EOPNOTSUPP)
> > +			dm_flush(md);
> >  	}
> >  
> >  	if (md->barrier_error != DM_ENDIO_REQUEUE)
> 
> 
> Doesn't store_barrier_error just record the result of the first empty
> barrier (not the -EOPNOTSUPP result of the unsupported discard)?
> 
> I'm missing how this change helps avoid the 2nd barrier for the
> -EOPNOTSUPP discard case.
> 
> ... And my testing shows that it doesn't.
> 
> Mike

Thanks for testing it. The errors of all the operations are accumulated in 
in md->barrier_error in dec_pending.

The problem was that it was ignoring -EOPNOTSUPP (assuming to ignore not 
supported empty barriers), but this condition unexpectedly ignored 
EOPNOTSUPP from the discard as well.

Please test with this patch.

Also, apply the patch to RHEL, because it is a bugfix (don't ignore 
discard errors).

Mikulas

---

dm barrier: A better test for -EOPNOTSUPP.

-EOPNOTSUPP could be generated only by empty barriers and we ignored that 
error, assuming that device not supporting cache flushes has cache always 
consistent.

With addition of discard barriers, this -EOPNOTSUPP could be generated by 
discards as well, and we can't ignore it.

This patch refines the test for -EOPNOTSUPP, ignoring it only for empty 
barrier requests.

Signed-off-by: Mikulas Patocka <mpatocka at redhat.com>

---
 drivers/md/dm.c |   15 +++++++++++++--
 1 file changed, 13 insertions(+), 2 deletions(-)

Index: linux-2.6.35-rc4-fast/drivers/md/dm.c
===================================================================
--- linux-2.6.35-rc4-fast.orig/drivers/md/dm.c	2010-07-08 01:52:21.000000000 +0200
+++ linux-2.6.35-rc4-fast/drivers/md/dm.c	2010-07-08 17:27:57.000000000 +0200
@@ -635,8 +635,14 @@ static void dec_pending(struct dm_io *io
 			 * There can be just one barrier request so we use
 			 * a per-device variable for error reporting.
 			 * Note that you can't touch the bio after end_io_acct
+			 *
+			 * We ignore -EOPNOTSUPP for empty flush reported by
+			 * underlying devices. We assume that if the device
+			 * doesn't support empty barriers, it doesn't need
+			 * cache flushing commands.
 			 */
-			if (!md->barrier_error && io_error != -EOPNOTSUPP)
+			if (!md->barrier_error &&
+			    !(bio_empty_barrier(bio) && io_error == -EOPNOTSUPP))
 				md->barrier_error = io_error;
 			end_io_acct(io);
 			free_io(md, io);