[PATCH] optimize one of the cache flushes

[PATCH] optimize one of the cache flushes

[Mikulas Patocka]

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@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)

Re: [PATCH] optimize one of the cache flushes

[Alasdair G Kergon]

On Wed, Jul 07, 2010 at 06:22:24PM -0400, Mikulas Patocka wrote:
>  	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);

Can we safely skip it with any other types of failures there too?
  
Alasdair

Re: optimize one of the cache flushes

[Mike Snitzer]

On Wed, Jul 07 2010 at  6:22pm -0400,
Mikulas Patocka <mpatocka@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@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

[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@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@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@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);

Re: dm barrier: A better test for -EOPNOTSUPP.

[Mike Snitzer]

On Thu, Jul 08 2010 at 11:52am -0400,
Mikulas Patocka <mpatocka@redhat.com> wrote:

> 
> 
> On Thu, 8 Jul 2010, Mike Snitzer wrote:
> > 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@redhat.com>

Works great, thanks.

Acked-by: Mike Snitzer <snitzer@redhat.com>