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

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

[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] 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

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

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