[PATCH, RFC] addjust discard request to be aligned with hwsect size to support SSDs with larger sector size

[PATCH, RFC] addjust discard request to be aligned with hwsect size to support SSDs with larger sector size

[Jiaying Zhang]

The currect blkdev_issue_discard() function assumes 512 sector size.
We have seen some problem when using discard on a SSD that has larger
sector size. The following patch adjusts the starting address and size of
a discard request to be aligned with hwsect size.

Signed-off-by: Jiaying Zhang <jiayingz@google.com>

diff --git a/block/blk-barrier.c b/block/blk-barrier.c
index 6d88544..576b7a1 100644
--- a/block/blk-barrier.c
+++ b/block/blk-barrier.c
@@ -376,14 +376,22 @@ int blkdev_issue_discard(struct block_device *bdev, sector_t sector,
 		DISCARD_BARRIER : DISCARD_NOBARRIER;
 	struct bio *bio;
 	struct page *page;
+	int hwsect_shift = blksize_bits(bdev_logical_block_size(bdev)) - 9;
+	int hwsect_mask = (1 << hwsect_shift) - 1;
+	sector_t end_sector;
 	int ret = 0;
 
 	if (!q)
 		return -ENXIO;
 
-	if (!blk_queue_discard(q))
+	if (!blk_queue_discard(q) || q->limits.max_discard_sectors == 0)
 		return -EOPNOTSUPP;
 
+	if (hwsect_shift > 0) {
+		end_sector = (sector + nr_sects) & ~hwsect_mask;
+		sector = (sector + hwsect_mask) & ~hwsect_mask;
+		nr_sects = end_sector - sector;
+	}
 	while (nr_sects && !ret) {
 		unsigned int sector_size = q->limits.logical_block_size;
 		unsigned int max_discard_sectors =

Re: [PATCH, RFC] addjust discard request to be aligned with hwsect size to support SSDs with larger sector size

[Christoph Hellwig]

On Wed, May 12, 2010 at 10:19:53PM -0700, Jiaying Zhang wrote:
> The currect blkdev_issue_discard() function assumes 512 sector size.
> We have seen some problem when using discard on a SSD that has larger
> sector size. The following patch adjusts the starting address and size of
> a discard request to be aligned with hwsect size.

Looks good,


Reviewed-by: Christoph Hellwig <hch@lst.de>

> -	if (!blk_queue_discard(q))
> +	if (!blk_queue_discard(q) || q->limits.max_discard_sectors == 0)
>  		return -EOPNOTSUPP;

But this change is unrelated.  It's fine with me, but needs a patch of
it's own with a proper description.

Re: [PATCH, RFC] addjust discard request to be aligned with hwsect size to support SSDs with larger sector size

[Martin K. Petersen]

>>>>> "Jiaying" == Jiaying Zhang <jiayingz@google.com> writes:

Jiaying,

Jiaying> The currect blkdev_issue_discard() function assumes 512 sector
Jiaying> size.

All of the block layer works on 512-byte sector units.  We don't
generally convert to logical blocks until we're in the disk driver.


Jiaying> We have seen some problem when using discard on a SSD that has
Jiaying> larger sector size. The following patch adjusts the starting
Jiaying> address and size of a discard request to be aligned with hwsect
Jiaying> size.

Jiaying> + int hwsect_shift =
Jiaying>  	blksize_bits(bdev_logical_block_size(bdev)) - 9;

Let's stop using the term hardware sector size.  It's an anachronism.

I guess I could understand if you aligned to the physical block size.
But the logical doesn't make much sense to me.

What is your logical block size?  Right now we only handle 512 bytes and
4KB in the discard path.

-- 
Martin K. Petersen	Oracle Linux Engineering

Re: [PATCH, RFC] addjust discard request to be aligned with hwsect size to support SSDs with larger sector size

[Jiaying Zhang]

On Sun, May 16, 2010 at 6:38 AM, Christoph Hellwig <hch@infradead.org> wrote:
> On Wed, May 12, 2010 at 10:19:53PM -0700, Jiaying Zhang wrote:
>> The currect blkdev_issue_discard() function assumes 512 sector size.
>> We have seen some problem when using discard on a SSD that has larger
>> sector size. The following patch adjusts the starting address and size of
>> a discard request to be aligned with hwsect size.
>
> Looks good,
>
>
> Reviewed-by: Christoph Hellwig <hch@lst.de>
>
>> -     if (!blk_queue_discard(q))
>> +     if (!blk_queue_discard(q) || q->limits.max_discard_sectors == 0)
>>               return -EOPNOTSUPP;
>
> But this change is unrelated.  It's fine with me, but needs a patch of
> it's own with a proper description.
>
I will break the patch into two.

Thanks!

Jiaying
>

Re: [PATCH, RFC] addjust discard request to be aligned with hwsect size to support SSDs with larger sector size

[Jiaying Zhang]

On Mon, May 17, 2010 at 11:57 AM, Martin K. Petersen
<martin.petersen@oracle.com> wrote:
>>>>>> "Jiaying" == Jiaying Zhang <jiayingz@google.com> writes:
>
Thanks a lot for the comments!

> Jiaying,
>
> Jiaying> The currect blkdev_issue_discard() function assumes 512 sector
> Jiaying> size.
>
> All of the block layer works on 512-byte sector units.  We don't
> generally convert to logical blocks until we're in the disk driver.
>
It is true that all of the block layer works on 512-byte sector size, but
I think it is good to check for address alignment for discard request so
we don't insert unnecessary discard requests into the request queue.
There are also certain disk drivers that assume a discard request
passed from the block layer is already properly aligned. We could
argue that those disk drivers need to fix that but I think it is better
that the block layer takes care of the address alignment so individual
disk drivers don't need to check that on their own.

>
> Jiaying> We have seen some problem when using discard on a SSD that has
> Jiaying> larger sector size. The following patch adjusts the starting
> Jiaying> address and size of a discard request to be aligned with hwsect
> Jiaying> size.
>
> Jiaying> + int hwsect_shift =
> Jiaying>        blksize_bits(bdev_logical_block_size(bdev)) - 9;
>
> Let's stop using the term hardware sector size.  It's an anachronism.
Ok. I will change those terms.

>
> I guess I could understand if you aligned to the physical block size.
> But the logical doesn't make much sense to me.
>
Physical block size seems better. I will change to use that.

> What is your logical block size?  Right now we only handle 512 bytes and
> 4KB in the discard path.
>
512 bytes and 4KB seem to be the most common sizes but I wouldn't be
surprised to see other logical block size.

Jiaying
> --
> Martin K. Petersen      Oracle Linux Engineering
>

Re: [PATCH, RFC] addjust discard request to be aligned with hwsect size to support SSDs with larger sector size

[Martin K. Petersen]

>>>>> "Jiaying" == Jiaying Zhang <jiayingz@google.com> writes:

Jiaying> It is true that all of the block layer works on 512-byte sector
Jiaying> size, but I think it is good to check for address alignment for
Jiaying> discard request so we don't insert unnecessary discard requests
Jiaying> into the request queue.

Well, then what happens when you stripe or use DM on devices with
different discard granularity?  Or what about a mirror?  Or multiple
levels of stacking of heterogeneous devices.

There are good reasons why we postpone the logical block scaling until
we're preparing the request for the actual physical device.


Jiaying> There are also certain disk drivers that assume a discard
Jiaying> request passed from the block layer is already properly
Jiaying> aligned. We could argue that those disk drivers need to fix
Jiaying> that

Which is what I'm arguing :)


Jiaying> 512 bytes and 4KB seem to be the most common sizes but I
Jiaying> wouldn't be surprised to see other logical block size.

That's the reason I'm asking.  If you have a different lbs then let's by
all means add support for it.  Or make the ULD scaling generic.

I'm open to aligning to the reported discard granularity in the ULD, for
instance.

-- 
Martin K. Petersen	Oracle Linux Engineering

Re: [PATCH, RFC] addjust discard request to be aligned with hwsect size to support SSDs with larger sector size

[Jiaying Zhang]

On Mon, May 17, 2010 at 1:00 PM, Martin K. Petersen
<martin.petersen@oracle.com> wrote:
>>>>>> "Jiaying" == Jiaying Zhang <jiayingz@google.com> writes:
>
> Jiaying> It is true that all of the block layer works on 512-byte sector
> Jiaying> size, but I think it is good to check for address alignment for
> Jiaying> discard request so we don't insert unnecessary discard requests
> Jiaying> into the request queue.
>
> Well, then what happens when you stripe or use DM on devices with
> different discard granularity?  Or what about a mirror?  Or multiple
> levels of stacking of heterogeneous devices.
>
> There are good reasons why we postpone the logical block scaling until
> we're preparing the request for the actual physical device.
>
Good point.

I am now open to either changing the patch to use discard_granularity
alignment or simply dropping the patch and leave the alignment checking
for disk drivers to deal with. Either way, disk driver needs to be changed.
Please let me know if you think using discard_granularity is the right
way to go. I will post a new patch in that case.

Jiaying

>
> Jiaying> There are also certain disk drivers that assume a discard
> Jiaying> request passed from the block layer is already properly
> Jiaying> aligned. We could argue that those disk drivers need to fix
> Jiaying> that
>
> Which is what I'm arguing :)
>
>
> Jiaying> 512 bytes and 4KB seem to be the most common sizes but I
> Jiaying> wouldn't be surprised to see other logical block size.
>
> That's the reason I'm asking.  If you have a different lbs then let's by
> all means add support for it.  Or make the ULD scaling generic.
>
> I'm open to aligning to the reported discard granularity in the ULD, for
> instance.
>
> --
> Martin K. Petersen      Oracle Linux Engineering
>