[PATCH] block: Force sector and nr_sects to device alignment and granularity.

[PATCH] block: Force sector and nr_sects to device alignment and granularity.

[Frank Mayhar]

block:  Force sector and nr_sects to device alignment and granularity.

In blkdev_issue_discard(), rather than sending an improperly-
aligned discard to the device (where it may get an error),
adjust the start and length to the block device alignment and
granularity.  Don't fail if this leaves nothing to discard.

Without this change, certain flash drivers can report invalid
trim parameters (and will fail the command).  Per tytso, "given
that discards are advisory, any part of the storage stack is
free to drop discard requests silently."

Signed-off-by: Frank Mayhar <fmayhar@google.com>
Reviewed-by: "Theodore Ts'o" <tytso@mit.edu>

 block/blk-lib.c | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/block/blk-lib.c b/block/blk-lib.c
index 97a733c..a4472cd 100644
--- a/block/blk-lib.c
+++ b/block/blk-lib.c
@@ -61,6 +61,21 @@ int blkdev_issue_discard(struct block_device *bdev,
sector_t sector,
 	alignment = (bdev_discard_alignment(bdev) >> 9) % granularity;
 
 	/*
+	 * Force sector and nr_sects to block device alignment and
+	 * granularity.
+	 */
+	if (alignment && (sector % alignment)) {
+		sector_t adj = alignment - (sector % alignment);
+
+		sector += adj;
+		nr_sects -= adj;
+	}
+	if (nr_sects % granularity)
+		nr_sects -= nr_sects % granularity;
+	if (!nr_sects)
+		return ret;
+
+	/*
 	 * Ensure that max_discard_sectors is of the proper
 	 * granularity, so that requests stay aligned after a split.
 	 */

Re: [PATCH] block: Force sector and nr_sects to device alignment and granularity.

[Jeff Moyer]

Frank Mayhar <fmayhar@google.com> writes:

> block:  Force sector and nr_sects to device alignment and granularity.
>
> In blkdev_issue_discard(), rather than sending an improperly-
> aligned discard to the device (where it may get an error),
> adjust the start and length to the block device alignment and
> granularity.  Don't fail if this leaves nothing to discard.
>
> Without this change, certain flash drivers can report invalid
> trim parameters (and will fail the command).  Per tytso, "given
> that discards are advisory, any part of the storage stack is
> free to drop discard requests silently."

And how do you get here with misaligned discards?

-Jeff

>
> Signed-off-by: Frank Mayhar <fmayhar@google.com>
> Reviewed-by: "Theodore Ts'o" <tytso@mit.edu>
>
>  block/blk-lib.c | 15 +++++++++++++++
>  1 file changed, 15 insertions(+)
>
> diff --git a/block/blk-lib.c b/block/blk-lib.c
> index 97a733c..a4472cd 100644
> --- a/block/blk-lib.c
> +++ b/block/blk-lib.c
> @@ -61,6 +61,21 @@ int blkdev_issue_discard(struct block_device *bdev,
> sector_t sector,
>  	alignment = (bdev_discard_alignment(bdev) >> 9) % granularity;
>  
>  	/*
> +	 * Force sector and nr_sects to block device alignment and
> +	 * granularity.
> +	 */
> +	if (alignment && (sector % alignment)) {
> +		sector_t adj = alignment - (sector % alignment);
> +
> +		sector += adj;
> +		nr_sects -= adj;
> +	}
> +	if (nr_sects % granularity)
> +		nr_sects -= nr_sects % granularity;
> +	if (!nr_sects)
> +		return ret;
> +
> +	/*
>  	 * Ensure that max_discard_sectors is of the proper
>  	 * granularity, so that requests stay aligned after a split.
>  	 */
>
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] block: Force sector and nr_sects to device alignment and granularity.

[Frank Mayhar]

On Tue, 2014-03-11 at 11:15 -0400, Jeff Moyer wrote:
> Frank Mayhar <fmayhar@google.com> writes:
> 
> > block:  Force sector and nr_sects to device alignment and granularity.
> >
> > In blkdev_issue_discard(), rather than sending an improperly-
> > aligned discard to the device (where it may get an error),
> > adjust the start and length to the block device alignment and
> > granularity.  Don't fail if this leaves nothing to discard.
> >
> > Without this change, certain flash drivers can report invalid
> > trim parameters (and will fail the command).  Per tytso, "given
> > that discards are advisory, any part of the storage stack is
> > free to drop discard requests silently."
> 
> And how do you get here with misaligned discards?

I don't understand the question.

The case that we were seeing was with an SSD that required TRIM on 8k
boundaries and with an 8k granularity.  Since the file system was trying
to do discards based on 4k alignment the driver complained mightily.
-- 
Frank Mayhar
310-460-4042

Re: [PATCH] block: Force sector and nr_sects to device alignment and granularity.

[Frank Mayhar]

On Mon, 2014-03-10 at 11:01 -0700, Frank Mayhar wrote:
> block:  Force sector and nr_sects to device alignment and granularity.

Just FYI, I'll be going out for surgery next week and recovery for a
couple of months after that.  If anyone has any further questions or
issues, either ask them this week or direct them to Ted T'so.
-- 
Frank Mayhar
310-460-4042

Re: [PATCH] block: Force sector and nr_sects to device alignment and granularity.

[Jeff Moyer]

Frank Mayhar <fmayhar@google.com> writes:

> On Tue, 2014-03-11 at 11:15 -0400, Jeff Moyer wrote:
>> Frank Mayhar <fmayhar@google.com> writes:
>> 
>> > block:  Force sector and nr_sects to device alignment and granularity.
>> >
>> > In blkdev_issue_discard(), rather than sending an improperly-
>> > aligned discard to the device (where it may get an error),
>> > adjust the start and length to the block device alignment and
>> > granularity.  Don't fail if this leaves nothing to discard.
>> >
>> > Without this change, certain flash drivers can report invalid
>> > trim parameters (and will fail the command).  Per tytso, "given
>> > that discards are advisory, any part of the storage stack is
>> > free to drop discard requests silently."
>> 
>> And how do you get here with misaligned discards?
>
> I don't understand the question.

Sorry if it wasn't clear...

> The case that we were seeing was with an SSD that required TRIM on 8k
> boundaries and with an 8k granularity.  Since the file system was trying
> to do discards based on 4k alignment the driver complained mightily.

but you managed to read my mind well enough.  The question is how high
up the stack do you put the logic for this?  Is it worth it to duplicate
the checks in the OS that are already done on the device?  I don't
know.  Martin, do you have an opinion on this?

-Jeff

Re: [PATCH] block: Force sector and nr_sects to device alignment and granularity.

[Frank Mayhar]

On Wed, 2014-03-12 at 14:20 -0400, Jeff Moyer wrote:
> but you managed to read my mind well enough.  The question is how high
> up the stack do you put the logic for this?  Is it worth it to duplicate
> the checks in the OS that are already done on the device?  I don't
> know.  Martin, do you have an opinion on this?

Well, my opinion (and I suspect that Ted agrees with me to at least some
extent) is that this is where it should be, i.e. in the block layer, in
the place that already knows about and deals with alignment and
granularity.  Sure, you could leave it to the device itself but it seems
reasonable to take care of this here for two reasons:  First, doing this
means that if a TRIM is issued it will be successful and the intent of
the discard will be at least partly satisfied.  Second, we're already
doing most of the computations and making decisions based on the
alignment and granularity anyway, so the overhead is pretty negligible
(and if the discard size goes to zero we short-circuit the process and
never go to the device at all).
-- 
Frank Mayhar
310-460-4042

Re: [PATCH] block: Force sector and nr_sects to device alignment and granularity.

[Jeff Moyer]

Frank Mayhar <fmayhar@google.com> writes:

> On Wed, 2014-03-12 at 14:20 -0400, Jeff Moyer wrote:
>> but you managed to read my mind well enough.  The question is how high
>> up the stack do you put the logic for this?  Is it worth it to duplicate
>> the checks in the OS that are already done on the device?  I don't
>> know.  Martin, do you have an opinion on this?
>
> Well, my opinion (and I suspect that Ted agrees with me to at least some
> extent) is that this is where it should be, i.e. in the block layer, in
> the place that already knows about and deals with alignment and
> granularity.  Sure, you could leave it to the device itself but it seems
> reasonable to take care of this here for two reasons:  First, doing this
> means that if a TRIM is issued it will be successful and the intent of

No, TRIM is advisory, even for well-formed TRIMs.  I guess you could
alter the definition of successful and have a correct statement there.

> the discard will be at least partly satisfied.  Second, we're already
> doing most of the computations and making decisions based on the
> alignment and granularity anyway, so the overhead is pretty negligible
> (and if the discard size goes to zero we short-circuit the process and
> never go to the device at all).

Sure, there's no sense getting information from the device and not using
it.  I guess you've talked me into it.

Cheers,
Jeff

Re: [PATCH] block: Force sector and nr_sects to device alignment and granularity.

[Frank Mayhar]

On Wed, 2014-03-12 at 15:33 -0400, Jeff Moyer wrote:
> No, TRIM is advisory, even for well-formed TRIMs.  I guess you could
> alter the definition of successful and have a correct statement there.

Yeah, you're right.  How about "is more likely to be successful."

> Sure, there's no sense getting information from the device and not using
> it.  I guess you've talked me into it.

In fact that was the gist of the local bug I fixed with this.  "The
driver has gone to the trouble of giving us this information, we should
use it."
-- 
Frank Mayhar
310-460-4042

Re: [PATCH] block: Force sector and nr_sects to device alignment and granularity.

[Martin K. Petersen]

>>>>> "Jeff" == Jeff Moyer <jmoyer@redhat.com> writes:

>> The case that we were seeing was with an SSD that required TRIM on 8k
>> boundaries and with an 8k granularity.  Since the file system was
>> trying to do discards based on 4k alignment the driver complained
>> mightily.

Jeff> but you managed to read my mind well enough.  The question is how
Jeff> high up the stack do you put the logic for this?  Is it worth it
Jeff> to duplicate the checks in the OS that are already done on the
Jeff> device?  I don't know.  Martin, do you have an opinion on this?

I'm no big fan of dropping information.

My original intent with the discard granularity and alignment was to
allow filesystems to use them to influence block allocation and layout.
Not to affect how we issue commands at runtime.

Since a storage device is free to ignore all or parts of any discard
request I'd consider it somewhat broken if it actually complained.
Especially so since the relevant knobs in the standard that we key off
of are performance recommendations and not requirements that commands
must adhere to.

-- 
Martin K. Petersen	Oracle Linux Engineering

Re: [PATCH] block: Force sector and nr_sects to device alignment and granularity.

[Frank Mayhar]

On Wed, 2014-03-12 at 21:47 -0400, Martin K. Petersen wrote:
> I'm no big fan of dropping information.
> 
> My original intent with the discard granularity and alignment was to
> allow filesystems to use them to influence block allocation and layout.
> Not to affect how we issue commands at runtime.
> 
> Since a storage device is free to ignore all or parts of any discard
> request I'd consider it somewhat broken if it actually complained.
> Especially so since the relevant knobs in the standard that we key off
> of are performance recommendations and not requirements that commands
> must adhere to.

Well, in this particular case the driver is filling in the relevant
information (alignment and granularity) and then complaining later that
that information has been ignored.  As I intimated earlier, it seems a
little odd to allow the driver to specify the information, only to
ignore it completely when it's time to actually use it.

Further, in my opinion this is less "dropping information" than it is
keeping information that would be dropped by the driver itself; were it
not for this adjustment, the driver would get the request, complain, and
drop it completely.  This way, as much of the request as possible is
preserved while still honoring the constraints given by the driver.
-- 
Frank Mayhar
310-460-4042

Re: [PATCH] block: Force sector and nr_sects to device alignment and granularity.

[Martin K. Petersen]

>>>>> "Frank" == Frank Mayhar <fmayhar@google.com> writes:

Frank,

Frank> Well, in this particular case the driver is filling in the
Frank> relevant information (alignment and granularity) and then
Frank> complaining later that that information has been ignored.  As I
Frank> intimated earlier, it seems a little odd to allow the driver to
Frank> specify the information, only to ignore it completely when it's
Frank> time to actually use it.

Which driver is this?

In T10 SBC these values are performance hints, not hard
requirements. They were never intended as such.

Frank> Further, in my opinion this is less "dropping information" than
Frank> it is keeping information that would be dropped by the driver
Frank> itself; were it not for this adjustment, the driver would get the
Frank> request, complain, and drop it completely.  This way, as much of
Frank> the request as possible is preserved while still honoring the
Frank> constraints given by the driver.

The problem arises if you combine devices with different
granularity. The I/O topology code is then forced to scale the
granularity up.

If you enforce the granularity at the top of the stack it means the
device(s) with lesser granularity will lose information which would
otherwise be valuable to them.

We have previously entertained enforcing the granularity at the bottom
of the stack on a per-device basis. However, I stand by my opinion that
the device behavior is broken. I'd never let a device like that pass
qualification here...

-- 
Martin K. Petersen	Oracle Linux Engineering