discard updates

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

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

Quoted 10 lines

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.

Quoted 6 lines

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

So yet another TODO item... Anyway:

Acked-by Mike Snitzer <[email protected]>

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

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

Index: linux-2.6.35-rc3-fast/include/linux/blkdev.h
===================================================================

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

Signed-off-by Mike Snitzer <[email protected]>

---
 include/linux/blkdev.h |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

include/linux/blkdev.h +1 -2

index baf5258..dbb510c 100644
@@ -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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Quoted 5 lines · 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.

Quoted 2 lines

Reported-by Mikulas Patocka <[email protected]>

Signed-off-by Mike Snitzer <[email protected]>

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?

Quoted 6 lines · Ted Ts'o

Quoted 5 lines · 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.

Quoted 8 lines

Quoted 2 lines

Reported-by Mikulas Patocka <[email protected]>

Signed-off-by Mike Snitzer <[email protected]>

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.

Quoted 38 lines · Mikulas Patocka

Quoted 10 lines

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.

Quoted 6 lines

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

So yet another TODO item... Anyway:

Acked-by Mike Snitzer <[email protected]>

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

Quoted 1 line

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

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.

Quoted 42 lines · Mikulas Patocka

Quoted 38 lines · Mikulas Patocka

Quoted 10 lines

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.

Quoted 6 lines

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

So yet another TODO item... Anyway:

Acked-by Mike Snitzer <[email protected]>

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

Quoted 5 lines

Quoted 1 line

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

Quoted 4 lines

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.

Quoted 11 lines

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

include/linux/blkdev.h static inline int sb_issue_discard(struc · line 1027

Quoted 8 lines

include/linux/blkdev.h +1 -1

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

Quoted 42 lines · Mikulas Patocka

Quoted 38 lines · Mikulas Patocka

Quoted 10 lines

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.

Quoted 6 lines

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

So yet another TODO item... Anyway:

Acked-by Mike Snitzer <[email protected]>

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

Quoted 5 lines

Quoted 1 line

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

Quoted 4 lines

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.

Quoted 15 lines

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:

Quoted 1 line

Acked-by Mike Snitzer <[email protected]>

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

Support discard if at least one underlying device supports it

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

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

Index: linux-2.6.35-rc3-fast/drivers/md/dm-table.c
===================================================================

drivers/md/dm-table.c +0 -0

@@ -911,12 +911,6 @@ int dm_table_complete(struct dm_table *t
 	int r = 0;
 	unsigned int leaf_nodes;

Support discard for multiple devices

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

Signed-off-by Mikulas Patocka <[email protected]>

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

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

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

Index: linux-2.6.35-rc3-fast/drivers/md/dm.c
===================================================================

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