[PATCH 0/4] discard updates

[PATCH 0/4] discard updates

[Mikulas Patocka]

Hi

Here are update patches for Mike's discard.

The first two patches bugfixes, they should be certainly applied to RHEL6.

The third patch supports discards on composite devices --- it could be 
considered a "bugfix" if not sending discards is considered a "bug". I 
think it still can go to RHEL6.

The fourth patch is an enhancement to support discard if only some of the 
underlying devices support it.

Mikulas

[PATCH 1/4] Check that the target supports discard

[Mikulas Patocka]

Check that the target supports discard

Because of a various race conditions, discard request may be submitted to
device mapper even if device mapper advertises that it doesn't support
discards. (for example, the device is linear, dm advertises discard support,
discard request is constructed, the table is reloaded with mirror, the discard
is submitted to the mirror target that doesn't support it).

To solve these problems we must check that the target supports discard
just before submitting discard request to it.

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

---
 drivers/md/dm.c |    3 +++
 1 file changed, 3 insertions(+)

Index: linux-2.6.35-rc3-fast/drivers/md/dm.c
===================================================================
--- linux-2.6.35-rc3-fast.orig/drivers/md/dm.c	2010-07-02 16:10:25.000000000 +0200
+++ linux-2.6.35-rc3-fast/drivers/md/dm.c	2010-07-02 16:11:22.000000000 +0200
@@ -1223,6 +1223,9 @@ static int __clone_and_map_discard(struc
 	if (!dm_target_is_valid(ti))
 		return -EIO;
 
+	if (!(ti->type->features & DM_TARGET_SUPPORTS_DISCARDS))
+		return -EOPNOTSUPP;
+
 	max = max_io_len(ci->md, ci->sector, ti);
 
 	if (ci->sector_count > max)

[PATCH 2/4] Clear the discard flag if the device loses discard capability

[Mikulas Patocka]

Clear the discard flag if the device loses discard capability
because of table reload.

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

---
 drivers/md/dm-table.c |    4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

Index: linux-2.6.35-rc3-fast/drivers/md/dm-table.c
===================================================================
--- linux-2.6.35-rc3-fast.orig/drivers/md/dm-table.c	2010-07-02 16:04:36.000000000 +0200
+++ linux-2.6.35-rc3-fast/drivers/md/dm-table.c	2010-07-02 16:07:49.000000000 +0200
@@ -1098,7 +1098,9 @@ void dm_table_set_restrictions(struct dm
 	else
 		queue_flag_set_unlocked(QUEUE_FLAG_CLUSTER, q);
 
-	if (dm_table_supports_discards(t))
+	if (!dm_table_supports_discards(t))
+		queue_flag_clear_unlocked(QUEUE_FLAG_DISCARD, q);
+	else
 		queue_flag_set_unlocked(QUEUE_FLAG_DISCARD, q);
 
 	dm_table_set_integrity(t);

[PATCH 3/4] Support discard for multiple devices

[Mikulas Patocka]

Support discard for multiple devices

The previous code supported discards only if there was one underlying device.
(i.e. multiple linear targets pointing to the same device would support
discards, multiple linear targets pointing to different devices wouldn't).

This restriction is not necessary, so this patch removes it.

As we checked, barrier+discard requests are handled by the barrier thread,
so it's safe to use these requests on devices with multiple underlying devices.

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

---
 drivers/md/dm-table.c |    6 ------
 1 file changed, 6 deletions(-)

Index: linux-2.6.35-rc3-fast/drivers/md/dm-table.c
===================================================================
--- linux-2.6.35-rc3-fast.orig/drivers/md/dm-table.c	2010-07-02 16:05:22.000000000 +0200
+++ linux-2.6.35-rc3-fast/drivers/md/dm-table.c	2010-07-02 16:07:45.000000000 +0200
@@ -911,12 +911,6 @@ int dm_table_complete(struct dm_table *t
 	int r = 0;
 	unsigned int leaf_nodes;
 
-	/*
-	 * We only support discards if there is exactly one underlying device.
-	 */
-	if (!list_is_singular(&t->devices))
-		t->discards_supported = 0;
-
 	/* how many indexes will the btree have ? */
 	leaf_nodes = dm_div_up(t->num_targets, KEYS_PER_NODE);
 	t->depth = 1 + int_log(leaf_nodes, CHILDREN_PER_NODE);

[PATCH 4/4] Support discard if at least one underlying device supports it

[Mikulas Patocka]

Support discard if at least one underlying device supports it

The previous code was too restrictive. It required that every underlying
device supports discard to advertise discard support.

If one of the underlying devices doesn't support discard and other does,
it makes still sense to advertise discard support and forward discards only
to the device that supports them.

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

---
 drivers/md/dm-table.c |   16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

Index: linux-2.6.35-rc3-fast/drivers/md/dm-table.c
===================================================================
--- linux-2.6.35-rc3-fast.orig/drivers/md/dm-table.c	2010-07-02 16:06:14.000000000 +0200
+++ linux-2.6.35-rc3-fast/drivers/md/dm-table.c	2010-07-02 16:07:32.000000000 +0200
@@ -1243,12 +1243,12 @@ struct mapped_device *dm_table_get_md(st
 	return t->md;
 }
 
-static int device_discard_incapable(struct dm_target *ti, struct dm_dev *dev,
-				    sector_t start, sector_t len, void *data)
+static int device_discard_capable(struct dm_target *ti, struct dm_dev *dev,
+				  sector_t start, sector_t len, void *data)
 {
 	struct request_queue *q = bdev_get_queue(dev->bdev);
 
-	return !q || !blk_queue_discard(q);
+	return q && blk_queue_discard(q);
 }
 
 bool dm_table_supports_discards(struct dm_table *t)
@@ -1260,7 +1260,7 @@ bool dm_table_supports_discards(struct d
 		return 0;
 
 	/*
-	 * Ensure underlying devices support discards.
+	 * Ensure that at least one underlying device supports discards.
 	 * t->devices includes internal dm devices such as mirror logs
 	 * so we need to use iterate_devices here, which targets
 	 * supporting discard must provide.
@@ -1268,12 +1268,12 @@ bool dm_table_supports_discards(struct d
 	while (i < dm_table_get_num_targets(t)) {
 		ti = dm_table_get_target(t, i++);
 
-		if (!ti->type->iterate_devices ||
-		    ti->type->iterate_devices(ti, device_discard_incapable, NULL))
-			return 0;
+		if (ti->type->iterate_devices &&
+		    ti->type->iterate_devices(ti, device_discard_capable, NULL))
+			return 1;
 	}
 
-	return 1;
+	return 0;
 }
 
 EXPORT_SYMBOL(dm_vcalloc);

Re: [PATCH 3/4] Support discard for multiple devices

[Mike Snitzer]

On Fri, Jul 02 2010 at 11:19am -0400,
Mikulas Patocka <mpatocka@redhat.com> wrote:

> Support discard for multiple devices
> 
> The previous code supported discards only if there was one underlying device.
> (i.e. multiple linear targets pointing to the same device would support
> discards, multiple linear targets pointing to different devices wouldn't).
> 
> This restriction is not necessary, so this patch removes it.
> 
> As we checked, barrier+discard requests are handled by the barrier thread,
> so it's safe to use these requests on devices with multiple underlying devices.
> 
> Signed-off-by: Mikulas Patocka <mpatocka@redhat.com>
> 
> ---
>  drivers/md/dm-table.c |    6 ------
>  1 file changed, 6 deletions(-)
> 
> Index: linux-2.6.35-rc3-fast/drivers/md/dm-table.c
> ===================================================================
> --- linux-2.6.35-rc3-fast.orig/drivers/md/dm-table.c	2010-07-02 16:05:22.000000000 +0200
> +++ linux-2.6.35-rc3-fast/drivers/md/dm-table.c	2010-07-02 16:07:45.000000000 +0200
> @@ -911,12 +911,6 @@ int dm_table_complete(struct dm_table *t
>  	int r = 0;
>  	unsigned int leaf_nodes;
>  
> -	/*
> -	 * We only support discards if there is exactly one underlying device.
> -	 */
> -	if (!list_is_singular(&t->devices))
> -		t->discards_supported = 0;
> -
>  	/* how many indexes will the btree have ? */
>  	leaf_nodes = dm_div_up(t->num_targets, KEYS_PER_NODE);
>  	t->depth = 1 + int_log(leaf_nodes, CHILDREN_PER_NODE);
> 

Removing this constraint means that a discard request that spans targets
will return -EOPNOTSUPP.

I'd prefer that we first make basic discard splitting work (like I
already have a DM patch to do that I'll rebase shortly).

But given the new-found desire for DM to return -EOPNOTSUPP as a means
to convey that a subset of the device does not support discards:

This change will start to force this issue with DM consumers higher up
the IO stack (e.g. ext4 and other filesystems).  So I'm cc'ing FS
development lists, if they don't care now they will at some point.

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

Re: [PATCH 2/4] Clear the discard flag if the device loses discard capability

[Mike Snitzer]

On Fri, Jul 02 2010 at 11:18am -0400,
Mikulas Patocka <mpatocka@redhat.com> wrote:

> Clear the discard flag if the device loses discard capability
> because of table reload.
> 
> Signed-off-by: Mikulas Patocka <mpatocka@redhat.com>

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

Re: [PATCH 1/4] Check that the target supports discard

[Mike Snitzer]

On Fri, Jul 02 2010 at 11:17am -0400,
Mikulas Patocka <mpatocka@redhat.com> wrote:

> Check that the target supports discard
> 
> Because of a various race conditions, discard request may be submitted to
> device mapper even if device mapper advertises that it doesn't support
> discards. (for example, the device is linear, dm advertises discard support,
> discard request is constructed, the table is reloaded with mirror, the discard
> is submitted to the mirror target that doesn't support it).
> 
> To solve these problems we must check that the target supports discard
> just before submitting discard request to it.
> 
> Signed-off-by: Mikulas Patocka <mpatocka@redhat.com>
> 
> ---
>  drivers/md/dm.c |    3 +++
>  1 file changed, 3 insertions(+)
> 
> Index: linux-2.6.35-rc3-fast/drivers/md/dm.c
> ===================================================================
> --- linux-2.6.35-rc3-fast.orig/drivers/md/dm.c	2010-07-02 16:10:25.000000000 +0200
> +++ linux-2.6.35-rc3-fast/drivers/md/dm.c	2010-07-02 16:11:22.000000000 +0200
> @@ -1223,6 +1223,9 @@ static int __clone_and_map_discard(struc
>  	if (!dm_target_is_valid(ti))
>  		return -EIO;
>  
> +	if (!(ti->type->features & DM_TARGET_SUPPORTS_DISCARDS))
> +		return -EOPNOTSUPP;
> +
>  	max = max_io_len(ci->md, ci->sector, ti);
>  
>  	if (ci->sector_count > max)
> 

The current model is that the static DM_TARGET_SUPPORTS_DISCARDS is
consulted when the target is added to table.  And then at the end of the
table load we check all devices to see if they actually do support
discards.

That said, I understand that IOs in flight before a table is suspended
will be attempted after the table is reload.  So this patch is good.

It should be noted that checking for this race condition gets much
harder once DM supports splitting discards that span devices.  We'll
have potential for partial completion that in the end returns
-EOPNOTSUPP.  Which is fine; but I think we'll need to be careful to not
return -EOPNOTSUPP before outstanding IO (associated with the discard
we're intending to return -EOPNOTSUPP for) completes?

Anyway, we'll cross that bridge later.  Nice catch.

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

Re: [PATCH 4/4] Support discard if at least one underlying device supports it

[Mike Snitzer]

On Fri, Jul 02 2010 at 11:19am -0400,
Mikulas Patocka <mpatocka@redhat.com> wrote:

> Support discard if at least one underlying device supports it
> 
> The previous code was too restrictive. It required that every underlying
> device supports discard to advertise discard support.
> 
> If one of the underlying devices doesn't support discard and other does,
> it makes still sense to advertise discard support and forward discards only
> to the device that supports them.
> 
> Signed-off-by: Mikulas Patocka <mpatocka@redhat.com>

As we discussed, we have a challenge where we need DM to avoid issuing
a barrier before the discard IFF a target doesn't support the discard
(which the barrier is paired with).

My understanding is that blkdev_issue_discard() only cares if the
discard was supported.  Barrier is used just to decorate the discard
(for correctness).  So by returning -EOPNOTSUPP we're saying the discard
isn't supported; we're not making any claims about the implict barrier,
so best to avoid the barrier entirely.

Otherwise we'll be issuing unnecessary barriers (and associated
performance loss).

So yet another TODO item... Anyway:

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

Re: [PATCH 4/4] Support discard if at least one underlying device supports it

[Mikulas Patocka]

> As we discussed, we have a challenge where we need DM to avoid issuing
> a barrier before the discard IFF a target doesn't support the discard
> (which the barrier is paired with).
> 
> My understanding is that blkdev_issue_discard() only cares if the
> discard was supported.  Barrier is used just to decorate the discard
> (for correctness).  So by returning -EOPNOTSUPP we're saying the discard
> isn't supported; we're not making any claims about the implict barrier,
> so best to avoid the barrier entirely.
> 
> Otherwise we'll be issuing unnecessary barriers (and associated
> performance loss).
> 
> So yet another TODO item... Anyway:
> 
> Acked-by: Mike Snitzer <snitzer@redhat.com>

Unnecessary barriers are issued anyway. With each freed extent.

The code must issue a "SYNCHRONIZE CACHE" to flush cache for previous 
writes, then "UNMAP" and then another "SYNCHRONIZE CACHE" to commit that 
unmap to disk. And this in loop for all extents in 
"release_blocks_on_commit".

One idea behind "discard barriers" was to submit a discard request and not 
wait for it. Then the request would need a barrier so that it doesn't get 
reordered with further writes (that may potentially write to the same area 
as the discarded area). But discard isn't used this way anyway, 
sb_issue_discard waits for completion, so the barrier isn't needed.

Even if ext4 developers wanted asynchronous discard requests, they should 
fire all the discards at once and then submit one zero-sized barrier. Not 
barrier with each discard request.

This is up to ext4 developers to optimize and remove the barriers and we 
can't do anything with it. Just send "SYNCHRONIZE 
CACHE"+"UNMAP"+"SYNCHRONIZE CACHE" like the barrier specification wants...

Mikulas

Re: [PATCH 4/4] Support discard if at least one underlying device supports it

[Mikulas Patocka]

On Fri, 2 Jul 2010, Mikulas Patocka wrote:

> > As we discussed, we have a challenge where we need DM to avoid issuing
> > a barrier before the discard IFF a target doesn't support the discard
> > (which the barrier is paired with).
> > 
> > My understanding is that blkdev_issue_discard() only cares if the
> > discard was supported.  Barrier is used just to decorate the discard
> > (for correctness).  So by returning -EOPNOTSUPP we're saying the discard
> > isn't supported; we're not making any claims about the implict barrier,
> > so best to avoid the barrier entirely.
> > 
> > Otherwise we'll be issuing unnecessary barriers (and associated
> > performance loss).
> > 
> > So yet another TODO item... Anyway:
> > 
> > Acked-by: Mike Snitzer <snitzer@redhat.com>
> 
> Unnecessary barriers are issued anyway. With each freed extent.
> 
> The code must issue a "SYNCHRONIZE CACHE" to flush cache for previous 
> writes, then "UNMAP" and then another "SYNCHRONIZE CACHE" to commit that 
> unmap to disk. And this in loop for all extents in 
> "release_blocks_on_commit".
> 
> One idea behind "discard barriers" was to submit a discard request and not 
> wait for it. Then the request would need a barrier so that it doesn't get 
> reordered with further writes (that may potentially write to the same area 
> as the discarded area). But discard isn't used this way anyway, 
> sb_issue_discard waits for completion, so the barrier isn't needed.
> 
> Even if ext4 developers wanted asynchronous discard requests, they should 
> fire all the discards at once and then submit one zero-sized barrier. Not 
> barrier with each discard request.
> 
> This is up to ext4 developers to optimize and remove the barriers and we 
> can't do anything with it. Just send "SYNCHRONIZE 
> CACHE"+"UNMAP"+"SYNCHRONIZE CACHE" like the barrier specification wants...
> 
> Mikulas

BTW. I understand that the current dm implementation will send two useless 
consecutive "SYNCHRONIZE CACHE" commands discard is directed to the part 
of the device that doesn't support it.

But the problem is that when you use discard on a part of the device that 
supports discard, it also sends two useless "SYNCHRONIZE CACHE" commands 
--- they are useless for functionality, but mandated by the barrier 
specification.

The fix is supposedly this:

---
 include/linux/blkdev.h |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Index: linux-2.6.35-rc3-fast/include/linux/blkdev.h
===================================================================
--- linux-2.6.35-rc3-fast.orig/include/linux/blkdev.h	2010-07-02 21:59:21.000000000 +0200
+++ linux-2.6.35-rc3-fast/include/linux/blkdev.h	2010-07-02 21:59:37.000000000 +0200
@@ -1021,7 +1021,7 @@ static inline int sb_issue_discard(struc
 	block <<= (sb->s_blocksize_bits - 9);
 	nr_blocks <<= (sb->s_blocksize_bits - 9);
 	return blkdev_issue_discard(sb->s_bdev, block, nr_blocks, GFP_KERNEL,
-				   BLKDEV_IFL_WAIT | BLKDEV_IFL_BARRIER);
+				   BLKDEV_IFL_WAIT);
 }
 
 extern int blk_verify_command(unsigned char *cmd, fmode_t has_write_perm);

GFP_KERNEL in ext4 (was: [PATCH 4/4] Support discard if at least one underlying device supports it)

[Mikulas Patocka]

> ---
>  include/linux/blkdev.h |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> Index: linux-2.6.35-rc3-fast/include/linux/blkdev.h
> ===================================================================
> --- linux-2.6.35-rc3-fast.orig/include/linux/blkdev.h	2010-07-02 21:59:21.000000000 +0200
> +++ linux-2.6.35-rc3-fast/include/linux/blkdev.h	2010-07-02 21:59:37.000000000 +0200
> @@ -1021,7 +1021,7 @@ static inline int sb_issue_discard(struc
>  	block <<= (sb->s_blocksize_bits - 9);
>  	nr_blocks <<= (sb->s_blocksize_bits - 9);
>  	return blkdev_issue_discard(sb->s_bdev, block, nr_blocks, GFP_KERNEL,
> 				   BLKDEV_IFL_WAIT | BLKDEV_IFL_BARRIER);
>  }
>  
>  extern int blk_verify_command(unsigned char *cmd, fmode_t has_write_perm);
> 

A note for ext4 developers: is that GFP_KERNEL safe? Can't it recurse back 
to ext4 and attempt to flush more data?

I'm not familiar enough with ext4 to declare that it is/isn't a bug, but 
it looks suspicious.

Mikulas

Re: [PATCH 4/4] Support discard if at least one underlying device supports it

[Mike Snitzer]

On Fri, Jul 02 2010 at  3:49pm -0400,
Mikulas Patocka <mpatocka@redhat.com> wrote:

> > As we discussed, we have a challenge where we need DM to avoid issuing
> > a barrier before the discard IFF a target doesn't support the discard
> > (which the barrier is paired with).
> > 
> > My understanding is that blkdev_issue_discard() only cares if the
> > discard was supported.  Barrier is used just to decorate the discard
> > (for correctness).  So by returning -EOPNOTSUPP we're saying the discard
> > isn't supported; we're not making any claims about the implict barrier,
> > so best to avoid the barrier entirely.
> > 
> > Otherwise we'll be issuing unnecessary barriers (and associated
> > performance loss).
> > 
> > So yet another TODO item... Anyway:
> > 
> > Acked-by: Mike Snitzer <snitzer@redhat.com>
> 
> Unnecessary barriers are issued anyway. With each freed extent.
>
> The code must issue a "SYNCHRONIZE CACHE" to flush cache for previous 
> writes, then "UNMAP" and then another "SYNCHRONIZE CACHE" to commit that 
> unmap to disk. And this in loop for all extents in 
> "release_blocks_on_commit".

You're delving into the mechanics of the discard when it is supported;
which is fine but tangential to my point above.  My point was DM
shouldn't issue any barrier(s) at all if it the discard will not be sent
(because a device doesn't support discards).

> One idea behind "discard barriers" was to submit a discard request and not 
> wait for it. Then the request would need a barrier so that it doesn't get 
> reordered with further writes (that may potentially write to the same area 
> as the discarded area). But discard isn't used this way anyway, 
> sb_issue_discard waits for completion, so the barrier isn't needed.
> 
> Even if ext4 developers wanted asynchronous discard requests, they should 
> fire all the discards at once and then submit one zero-sized barrier. Not 
> barrier with each discard request.

sb_issue_discard() is the block layer api that ext4 uses for discards.
Ext4, or any other filesystem that uses sb_issue_discard(), has no
control over the barriers that are issued.
 
> This is up to ext4 developers to optimize and remove the barriers and we 
> can't do anything with it. Just send "SYNCHRONIZE 
> CACHE"+"UNMAP"+"SYNCHRONIZE CACHE" like the barrier specification wants...

In practice that is what I see when I remove a file in ext4:

        kdmflush-2537  [000] 911436.484481: scsi_dispatch_cmd_start: host_no=5 channel=0 id=0 lun=0 data_sgl=0 prot_sgl=0 cmnd=(SYNCHRONIZE_CACHE - raw=35 00 00 00 00 00 00 00 00 00)
        kdmflush-2537  [000] 911436.484482: scsi_dispatch_cmd_done: host_no=5 channel=0 id=0 lun=0 data_sgl=0 prot_sgl=0 cmnd=(SYNCHRONIZE_CACHE - raw=35 00 00 00 00 00 00 00 00 00) result=(driver=DRIVER_OK host=DID_OK message=COMMAND_COMPLETE status=SAM_STAT_GOOD)

        kdmflush-2537  [000] 911436.484500: scsi_dispatch_cmd_start: host_no=5 channel=0 id=0 lun=0 data_sgl=1 prot_sgl=0 cmnd=(UNMAP regions=1 raw=42 00 00 00 00 00 00 00 18 00)
          <idle>-0     [000] 911436.485238: scsi_dispatch_cmd_done: host_no=5 channel=0 id=0 lun=0 data_sgl=1 prot_sgl=0 cmnd=(UNMAP regions=1 raw=42 00 00 00 00 00 00 00 18 00) result=(driver=DRIVER_OK host=DID_OK message=COMMAND_COMPLETE status=SAM_STAT_GOOD)

        kdmflush-2537  [000] 911436.485283: scsi_dispatch_cmd_start: host_no=5 channel=0 id=0 lun=0 data_sgl=0 prot_sgl=0 cmnd=(SYNCHRONIZE_CACHE - raw=35 00 00 00 00 00 00 00 00 00)
        kdmflush-2537  [000] 911436.485284: scsi_dispatch_cmd_done: host_no=5 channel=0 id=0 lun=0 data_sgl=0 prot_sgl=0 cmnd=(SYNCHRONIZE_CACHE - raw=35 00 00 00 00 00 00 00 00 00) result=(driver=DRIVER_OK host=DID_OK message=COMMAND_COMPLETE status=SAM_STAT_GOOD)

Mike

Re: [PATCH 4/4] Support discard if at least one underlying device supports it

[Mike Snitzer]

On Fri, Jul 02 2010 at  4:00pm -0400,
Mikulas Patocka <mpatocka@redhat.com> wrote:

> 
> 
> On Fri, 2 Jul 2010, Mikulas Patocka wrote:
> 
> > > As we discussed, we have a challenge where we need DM to avoid issuing
> > > a barrier before the discard IFF a target doesn't support the discard
> > > (which the barrier is paired with).
> > > 
> > > My understanding is that blkdev_issue_discard() only cares if the
> > > discard was supported.  Barrier is used just to decorate the discard
> > > (for correctness).  So by returning -EOPNOTSUPP we're saying the discard
> > > isn't supported; we're not making any claims about the implict barrier,
> > > so best to avoid the barrier entirely.
> > > 
> > > Otherwise we'll be issuing unnecessary barriers (and associated
> > > performance loss).
> > > 
> > > So yet another TODO item... Anyway:
> > > 
> > > Acked-by: Mike Snitzer <snitzer@redhat.com>
> > 
> > Unnecessary barriers are issued anyway. With each freed extent.
> > 
> > The code must issue a "SYNCHRONIZE CACHE" to flush cache for previous 
> > writes, then "UNMAP" and then another "SYNCHRONIZE CACHE" to commit that 
> > unmap to disk. And this in loop for all extents in 
> > "release_blocks_on_commit".
> > 
> > One idea behind "discard barriers" was to submit a discard request and not 
> > wait for it. Then the request would need a barrier so that it doesn't get 
> > reordered with further writes (that may potentially write to the same area 
> > as the discarded area). But discard isn't used this way anyway, 
> > sb_issue_discard waits for completion, so the barrier isn't needed.
> > 
> > Even if ext4 developers wanted asynchronous discard requests, they should 
> > fire all the discards at once and then submit one zero-sized barrier. Not 
> > barrier with each discard request.
> > 
> > This is up to ext4 developers to optimize and remove the barriers and we 
> > can't do anything with it. Just send "SYNCHRONIZE 
> > CACHE"+"UNMAP"+"SYNCHRONIZE CACHE" like the barrier specification wants...
> > 
> > Mikulas
> 
> BTW. I understand that the current dm implementation will send two useless 
> consecutive "SYNCHRONIZE CACHE" commands discard is directed to the part 
> of the device that doesn't support it.

Issue 1 ^^^

> But the problem is that when you use discard on a part of the device that 
> supports discard, it also sends two useless "SYNCHRONIZE CACHE" commands 
> --- they are useless for functionality, but mandated by the barrier 
> specification.

Issue 2 ^^^

Those are 2 different issues.  Please don't join them as if they are one
in the same.  DM should treat a discard as a first class request (which
may or may not have a barrier).  If a region doesn't support the discard
DM has no business processing anything related to the discard (barriers
included).  It is as simple as that.

> The fix is supposedly this:
> 
> ---
>  include/linux/blkdev.h |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> Index: linux-2.6.35-rc3-fast/include/linux/blkdev.h
> ===================================================================
> --- linux-2.6.35-rc3-fast.orig/include/linux/blkdev.h	2010-07-02 21:59:21.000000000 +0200
> +++ linux-2.6.35-rc3-fast/include/linux/blkdev.h	2010-07-02 21:59:37.000000000 +0200
> @@ -1021,7 +1021,7 @@ static inline int sb_issue_discard(struc
>  	block <<= (sb->s_blocksize_bits - 9);
>  	nr_blocks <<= (sb->s_blocksize_bits - 9);
>  	return blkdev_issue_discard(sb->s_bdev, block, nr_blocks, GFP_KERNEL,
> -				   BLKDEV_IFL_WAIT | BLKDEV_IFL_BARRIER);
> +				   BLKDEV_IFL_WAIT);
>  }
>  
>  extern int blk_verify_command(unsigned char *cmd, fmode_t has_write_perm);

Hmm, older kernels use DISCARD_FL_BARRIER which merely mapped to
BLKDEV_IFL_BARRIER.

Seems you've stumbled onto a bug in the conversion that commit
"blkdev: generalize flags for blkdev_issue_fn functions"
(fbd9b09a177a481eda) performed?

That commit seems to have incorrectly replaced DISCARD_FL_BARRIER with
both: BLKDEV_IFL_WAIT | BLKDEV_IFL_BARRIER

Dmitry and/or Jens was this intended?

Mike

Re: [PATCH 4/4] Support discard if at least one underlying device supports it

[Alasdair G Kergon]

On Fri, Jul 02, 2010 at 04:47:09PM -0400, Mike Snitzer wrote:

> If a region doesn't support the discard
> DM has no business processing anything related to the discard (barriers
> included).  It is as simple as that.
 
Indeed - if an I/O is going to fail, discover that as early as we can, trying
to avoid the relatively-expensive barrier process whenever we reasonably can.

Alasdair

Re: [PATCH 4/4] Support discard if at least one underlying device supports it

[Dmitry Monakhov]

Mike Snitzer <snitzer@redhat.com> writes:

> On Fri, Jul 02 2010 at  4:00pm -0400,
> Mikulas Patocka <mpatocka@redhat.com> wrote:
>
>> 
>> 
>> On Fri, 2 Jul 2010, Mikulas Patocka wrote:
>> 
>> > > As we discussed, we have a challenge where we need DM to avoid issuing
>> > > a barrier before the discard IFF a target doesn't support the discard
>> > > (which the barrier is paired with).
>> > > 
>> > > My understanding is that blkdev_issue_discard() only cares if the
>> > > discard was supported.  Barrier is used just to decorate the discard
>> > > (for correctness).  So by returning -EOPNOTSUPP we're saying the discard
>> > > isn't supported; we're not making any claims about the implict barrier,
>> > > so best to avoid the barrier entirely.
>> > > 
>> > > Otherwise we'll be issuing unnecessary barriers (and associated
>> > > performance loss).
>> > > 
>> > > So yet another TODO item... Anyway:
>> > > 
>> > > Acked-by: Mike Snitzer <snitzer@redhat.com>
>> > 
>> > Unnecessary barriers are issued anyway. With each freed extent.
>> > 
>> > The code must issue a "SYNCHRONIZE CACHE" to flush cache for previous 
>> > writes, then "UNMAP" and then another "SYNCHRONIZE CACHE" to commit that 
>> > unmap to disk. And this in loop for all extents in 
>> > "release_blocks_on_commit".
>> > 
>> > One idea behind "discard barriers" was to submit a discard request and not 
>> > wait for it. Then the request would need a barrier so that it doesn't get 
>> > reordered with further writes (that may potentially write to the same area 
>> > as the discarded area). But discard isn't used this way anyway, 
>> > sb_issue_discard waits for completion, so the barrier isn't needed.
>> > 
>> > Even if ext4 developers wanted asynchronous discard requests, they should 
>> > fire all the discards at once and then submit one zero-sized barrier. Not 
>> > barrier with each discard request.
>> > 
>> > This is up to ext4 developers to optimize and remove the barriers and we 
>> > can't do anything with it. Just send "SYNCHRONIZE 
>> > CACHE"+"UNMAP"+"SYNCHRONIZE CACHE" like the barrier specification wants...
>> > 
>> > Mikulas
>> 
>> BTW. I understand that the current dm implementation will send two useless 
>> consecutive "SYNCHRONIZE CACHE" commands discard is directed to the part 
>> of the device that doesn't support it.
>
> Issue 1 ^^^
>
>> But the problem is that when you use discard on a part of the device that 
>> supports discard, it also sends two useless "SYNCHRONIZE CACHE" commands 
>> --- they are useless for functionality, but mandated by the barrier 
>> specification.
>
> Issue 2 ^^^
>
> Those are 2 different issues.  Please don't join them as if they are one
> in the same.  DM should treat a discard as a first class request (which
> may or may not have a barrier).  If a region doesn't support the discard
> DM has no business processing anything related to the discard (barriers
> included).  It is as simple as that.
>
>> The fix is supposedly this:
>> 
>> ---
>>  include/linux/blkdev.h |    2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>> 
>> Index: linux-2.6.35-rc3-fast/include/linux/blkdev.h
>> ===================================================================
>> --- linux-2.6.35-rc3-fast.orig/include/linux/blkdev.h	2010-07-02 21:59:21.000000000 +0200
>> +++ linux-2.6.35-rc3-fast/include/linux/blkdev.h	2010-07-02 21:59:37.000000000 +0200
>> @@ -1021,7 +1021,7 @@ static inline int sb_issue_discard(struc
>>  	block <<= (sb->s_blocksize_bits - 9);
>>  	nr_blocks <<= (sb->s_blocksize_bits - 9);
>>  	return blkdev_issue_discard(sb->s_bdev, block, nr_blocks, GFP_KERNEL,
>> -				   BLKDEV_IFL_WAIT | BLKDEV_IFL_BARRIER);
>> +				   BLKDEV_IFL_WAIT);
>>  }
>>  
>>  extern int blk_verify_command(unsigned char *cmd, fmode_t has_write_perm);
>
> Hmm, older kernels use DISCARD_FL_BARRIER which merely mapped to
> BLKDEV_IFL_BARRIER.
>
> Seems you've stumbled onto a bug in the conversion that commit
> "blkdev: generalize flags for blkdev_issue_fn functions"
> (fbd9b09a177a481eda) performed?
>
> That commit seems to have incorrectly replaced DISCARD_FL_BARRIER with
> both: BLKDEV_IFL_WAIT | BLKDEV_IFL_BARRIER
>
> Dmitry and/or Jens was this intended?
Yes, before the path WAIT behavior was implicit, now caller is
responsible for exact behavior.
So, as it was mentioned earlier, it is reasonable to send
discard request only with BLKDEV_IFL_BARRIER flag from some places in
ext4. I have optimization patches for that in my queue, i hope they
will be ready soon.
>
> Mike

Re: [PATCH 4/4] Support discard if at least one underlying device supports it

[Mikulas Patocka]

On Fri, 2 Jul 2010, Mike Snitzer wrote:

> On Fri, Jul 02 2010 at  4:00pm -0400,
> Mikulas Patocka <mpatocka@redhat.com> wrote:
> 
> > 
> > 
> > On Fri, 2 Jul 2010, Mikulas Patocka wrote:
> > 
> > > > As we discussed, we have a challenge where we need DM to avoid issuing
> > > > a barrier before the discard IFF a target doesn't support the discard
> > > > (which the barrier is paired with).
> > > > 
> > > > My understanding is that blkdev_issue_discard() only cares if the
> > > > discard was supported.  Barrier is used just to decorate the discard
> > > > (for correctness).  So by returning -EOPNOTSUPP we're saying the discard
> > > > isn't supported; we're not making any claims about the implict barrier,
> > > > so best to avoid the barrier entirely.
> > > > 
> > > > Otherwise we'll be issuing unnecessary barriers (and associated
> > > > performance loss).
> > > > 
> > > > So yet another TODO item... Anyway:
> > > > 
> > > > Acked-by: Mike Snitzer <snitzer@redhat.com>
> > > 
> > > Unnecessary barriers are issued anyway. With each freed extent.
> > > 
> > > The code must issue a "SYNCHRONIZE CACHE" to flush cache for previous 
> > > writes, then "UNMAP" and then another "SYNCHRONIZE CACHE" to commit that 
> > > unmap to disk. And this in loop for all extents in 
> > > "release_blocks_on_commit".
> > > 
> > > One idea behind "discard barriers" was to submit a discard request and not 
> > > wait for it. Then the request would need a barrier so that it doesn't get 
> > > reordered with further writes (that may potentially write to the same area 
> > > as the discarded area). But discard isn't used this way anyway, 
> > > sb_issue_discard waits for completion, so the barrier isn't needed.
> > > 
> > > Even if ext4 developers wanted asynchronous discard requests, they should 
> > > fire all the discards at once and then submit one zero-sized barrier. Not 
> > > barrier with each discard request.
> > > 
> > > This is up to ext4 developers to optimize and remove the barriers and we 
> > > can't do anything with it. Just send "SYNCHRONIZE 
> > > CACHE"+"UNMAP"+"SYNCHRONIZE CACHE" like the barrier specification wants...
> > > 
> > > Mikulas
> > 
> > BTW. I understand that the current dm implementation will send two useless 
> > consecutive "SYNCHRONIZE CACHE" commands discard is directed to the part 
> > of the device that doesn't support it.
> 
> Issue 1 ^^^
> 
> > But the problem is that when you use discard on a part of the device that 
> > supports discard, it also sends two useless "SYNCHRONIZE CACHE" commands 
> > --- they are useless for functionality, but mandated by the barrier 
> > specification.
> 
> Issue 2 ^^^
> 
> Those are 2 different issues.  Please don't join them as if they are one
> in the same.  DM should treat a discard as a first class request (which
> may or may not have a barrier).

What I mean --- if you fix Issue 2, Issue 1 is no longer a problem.

> If a region doesn't support the discard
> DM has no business processing anything related to the discard (barriers
> included).  It is as simple as that.

You can optimize out the second SYNCHRONIZE CACHE, but not the first one 
(because when it is sent, we don't know if the discard will succeed or 
not).

Basically, the fix is to prefix the second dm_flush in process_barrier 
with if (md->barrier_error != -EOPNOTSUPP).

The "barrier+discard" concept is problematic anyway. If we specify that 
"barrier+discard" request doesn't have to do the barrier if discard fails 
(as it is currently), then the request is useless to maintain disk 
integrity because the discard may fail anytime (and so the barrier).

I think they will eventually remove "barrier+discard" from the filesystems 
at all.

Mikulas

[PATCH] disallow FS recursion from sb_issue_discard allocation

[Mike Snitzer]

Filesystems can call sb_issue_discard on a memory reclaim path
(e.g. ext4 calls sb_issue_discard during journal commit).

Use GFP_NOFS in sb_issue_discard to avoid recursing back into the FS.

Reported-by: Mikulas Patocka <mpatocka@redhat.com>
Signed-off-by: Mike Snitzer <snitzer@redhat.com>
---
 include/linux/blkdev.h |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h
index baf5258..dbb510c 100644
--- a/include/linux/blkdev.h
+++ b/include/linux/blkdev.h
@@ -933,7 +933,7 @@ static inline int sb_issue_discard(struct super_block *sb,
 {
 	block <<= (sb->s_blocksize_bits - 9);
 	nr_blocks <<= (sb->s_blocksize_bits - 9);
-	return blkdev_issue_discard(sb->s_bdev, block, nr_blocks, GFP_KERNEL,
+	return blkdev_issue_discard(sb->s_bdev, block, nr_blocks, GFP_NOFS,
 				   BLKDEV_IFL_WAIT | BLKDEV_IFL_BARRIER);
 }
 

Re: [PATCH 1/4] Check that the target supports discard

[Alasdair G Kergon]

On Fri, Jul 02, 2010 at 11:17:52AM -0400, Mikulas Patocka wrote:
> To solve these problems we must check that the target supports discard
> just before submitting discard request to it.
 
Folded into the earlier patch,

dm-linear-support-discard.patch

Alasdair

Re: [PATCH 2/4] Clear the discard flag if the device loses discard capability

[Alasdair G Kergon]

On Fri, Jul 02, 2010 at 11:18:29AM -0400, Mikulas Patocka wrote:
> Clear the discard flag if the device loses discard capability
> because of table reload.
 
Folded into original patch.
dm-linear-support-discard.patch

Alasdair

Re: [PATCH 3/4] Support discard for multiple devices

[Alasdair G Kergon]

On Fri, Jul 02, 2010 at 11:19:02AM -0400, Mikulas Patocka wrote:
> Support discard for multiple devices
 
Folded into original patch.
dm-linear-support-discard.patch

Alasdair

Re: [PATCH 4/4] Support discard if at least one underlying device supports it

[Alasdair G Kergon]

On Fri, Jul 02, 2010 at 11:19:53AM -0400, Mikulas Patocka wrote:
> Support discard if at least one underlying device supports it
 
Folded into original patch.
dm-linear-support-discard.patch

Alasdair

Re: [PATCH] disallow FS recursion from sb_issue_discard allocation

[Ted Ts'o]

On Tue, Jul 06, 2010 at 12:11:56PM -0400, Mike Snitzer wrote:
> Filesystems can call sb_issue_discard on a memory reclaim path
> (e.g. ext4 calls sb_issue_discard during journal commit).
> 
> Use GFP_NOFS in sb_issue_discard to avoid recursing back into the FS.
> 
> Reported-by: Mikulas Patocka <mpatocka@redhat.com>
> Signed-off-by: Mike Snitzer <snitzer@redhat.com>

Hi Jens,

I never saw an ack from you on this patch.  Are you ok with it, and
have you grabbed it for your tree?  Do you want me to include this in
the ext4 tree, even though it's a patch to include/linux/blkdev.h?

Thanks,

						- Ted


> ---
>  include/linux/blkdev.h |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h
> index baf5258..dbb510c 100644
> --- a/include/linux/blkdev.h
> +++ b/include/linux/blkdev.h
> @@ -933,7 +933,7 @@ static inline int sb_issue_discard(struct super_block *sb,
>  {
>  	block <<= (sb->s_blocksize_bits - 9);
>  	nr_blocks <<= (sb->s_blocksize_bits - 9);
> -	return blkdev_issue_discard(sb->s_bdev, block, nr_blocks, GFP_KERNEL,
> +	return blkdev_issue_discard(sb->s_bdev, block, nr_blocks, GFP_NOFS,
>  				   BLKDEV_IFL_WAIT | BLKDEV_IFL_BARRIER);
>  }
>  
> --
> To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: disallow FS recursion from sb_issue_discard allocation

[Mike Snitzer]

On Tue, Jul 27 2010 at  9:44am -0400,
Ted Ts'o <tytso@mit.edu> wrote:

> On Tue, Jul 06, 2010 at 12:11:56PM -0400, Mike Snitzer wrote:
> > Filesystems can call sb_issue_discard on a memory reclaim path
> > (e.g. ext4 calls sb_issue_discard during journal commit).
> > 
> > Use GFP_NOFS in sb_issue_discard to avoid recursing back into the FS.
> > 
> > Reported-by: Mikulas Patocka <mpatocka@redhat.com>
> > Signed-off-by: Mike Snitzer <snitzer@redhat.com>
> 
> Hi Jens,
> 
> I never saw an ack from you on this patch.  Are you ok with it, and
> have you grabbed it for your tree?  Do you want me to include this in
> the ext4 tree, even though it's a patch to include/linux/blkdev.h?

Hi Ted,

Thanks for following up on this.  In my experience, Jens is more apt to
pick up a patch if it gets explicitly 'Acked-by' other stake-holders
(especially when a patch is motivated by another subsystem, in this case
the proposed block change addresses a problem unique to fs/ext4).

Mike

Re: disallow FS recursion from sb_issue_discard allocation

[Jens Axboe]

On 07/27/2010 05:33 PM, Mike Snitzer wrote:
> On Tue, Jul 27 2010 at  9:44am -0400,
> Ted Ts'o <tytso@mit.edu> wrote:
> 
>> On Tue, Jul 06, 2010 at 12:11:56PM -0400, Mike Snitzer wrote:
>>> Filesystems can call sb_issue_discard on a memory reclaim path
>>> (e.g. ext4 calls sb_issue_discard during journal commit).
>>>
>>> Use GFP_NOFS in sb_issue_discard to avoid recursing back into the FS.
>>>
>>> Reported-by: Mikulas Patocka <mpatocka@redhat.com>
>>> Signed-off-by: Mike Snitzer <snitzer@redhat.com>
>>
>> Hi Jens,
>>
>> I never saw an ack from you on this patch.  Are you ok with it, and
>> have you grabbed it for your tree?  Do you want me to include this in
>> the ext4 tree, even though it's a patch to include/linux/blkdev.h?
> 
> Hi Ted,
> 
> Thanks for following up on this.  In my experience, Jens is more apt to
> pick up a patch if it gets explicitly 'Acked-by' other stake-holders
> (especially when a patch is motivated by another subsystem, in this case
> the proposed block change addresses a problem unique to fs/ext4).

I'll pick this up. I've been away for a few weeks and I'm currently
on vacation, but I'll push this with a few other pending patches
for .35 on monday.

-- 
Jens Axboe


Confidentiality Notice: This e-mail message, its contents and any attachments to it are confidential to the intended recipient, and may contain information that is privileged and/or exempt from disclosure under applicable law. If you are not the intended recipient, please immediately notify the sender and destroy the original e-mail message and any attachments (and any copies that may have been made) from your system or otherwise. Any unauthorized use, copying, disclosure or distribution of this information is strictly prohibited.

Re: disallow FS recursion from sb_issue_discard allocation

[Ted Ts'o]

On Wed, Jul 28, 2010 at 08:12:15PM +0200, Jens Axboe wrote:
> 
> I'll pick this up. I've been away for a few weeks and I'm currently
> on vacation, but I'll push this with a few other pending patches
> for .35 on monday.

Great, thanks!!

					- Ted