On Fri, Nov 02, 2012 at 09:40:58AM +0800, Shaohua Li wrote:

On Thu, Nov 01, 2012 at 05:38:54PM +1100, NeilBrown wrote:

quoted

Hi Shaohua,
 I've been doing some testing and discovered a problem with your discard
 support for RAID5.

 The code in blkdev_issue_discard assumes that the 'granularity' is a power
 of 2, and for example subtracts 1 to get a mask.

 However RAID5 sets the granularity to be the stripe size which often is not
 a power of two.  When this happens you can easily get into an infinite loop.

 I suspect that to make this work properly, blkdev_issue_discard will need to
 be changed to allow 'granularity' to be an arbitrary value.
 When it is a power of two, the current masking can be used.
 When it is anything else, it will need to use sector_div().

Yep, looks we need use sector_div. And this isn't the only problem. discard
request can be merged, and the merge check only checks max_discard_sectors.
That means the split requests in blkdev_issue_discard can be merged again. The
split nerver works.

I'm wondering what's purpose of discard_alignment and discard_granularity. Are
there devices with discard_granularity not 1 sector?

Most certainly. Thin provisioned storage often has granularity in the
order of megabytes....

If bio isn't discard
aligned, what device will do?

Up to the device.

Further, why driver handles alignment/granularity
if device will ignore misaligned request.

When you send a series of sequential unaligned requests, the device
may ignore them all. Hence you end up with nothing being discarded,
even though the entire range being discarded is much, much larger
than the discard granularity....

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com