Re: [PATCH 2/2] blkdiscard: fail on sector misalignment

["Lukáš Czerner" <lczerner@redhat.com>]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 2951 bytes --]

On Fri, 24 Oct 2014, Federico Simoncelli wrote:

> Date: Fri, 24 Oct 2014 05:47:04 -0400 (EDT)
> From: Federico Simoncelli <fsimonce@redhat.com>
> To: Lukáš Czerner <lczerner@redhat.com>
> Cc: util-linux@vger.kernel.org, kzak@redhat.com
> Subject: Re: [PATCH 2/2] blkdiscard: fail on sector misalignment
> 
> ----- Original Message -----
> > From: "Lukáš Czerner" <lczerner@redhat.com>
> > To: "Federico Simoncelli" <fsimonce@redhat.com>
> > Cc: util-linux@vger.kernel.org, kzak@redhat.com
> > Sent: Friday, October 24, 2014 11:37:33 AM
> > Subject: Re: [PATCH 2/2] blkdiscard: fail on sector misalignment
> > 
> > On Thu, 23 Oct 2014, Federico Simoncelli wrote:
> > 
> > > Date: Thu, 23 Oct 2014 15:34:36 +0000
> > > From: Federico Simoncelli <fsimonce@redhat.com>
> > > To: util-linux@vger.kernel.org
> > > Cc: kzak@redhat.com, lczerner@redhat.com,
> > >     Federico Simoncelli <fsimonce@redhat.com>
> > > Subject: [PATCH 2/2] blkdiscard: fail on sector misalignment
> > > 
> > > ---
> > >  sys-utils/blkdiscard.c            | 10 +++++++---
> > >  tests/expected/blkdiscard/offsets | 12 ++++++------
> > >  2 files changed, 13 insertions(+), 9 deletions(-)
> > > 
> > > diff --git a/sys-utils/blkdiscard.c b/sys-utils/blkdiscard.c
> > > index 1eb2b285..76c72b8 100644
> > > --- a/sys-utils/blkdiscard.c
> > > +++ b/sys-utils/blkdiscard.c
> > > @@ -144,9 +144,9 @@ int main(int argc, char **argv)
> > >  	if (ioctl(fd, BLKSSZGET, &secsize))
> > >  		err(EXIT_FAILURE, _("%s: BLKSSZGET ioctl failed"), path);
> > >  
> > > -	/* align range to the sector size */
> > > -	range[0] = (range[0] + secsize - 1) & ~(secsize - 1);
> > > -	range[1] &= ~(secsize - 1);
> > > +	/* check offset alignment to the sector size */
> > > +	if (range[0] % secsize)
> > > +		errx(EXIT_FAILURE, _("%s: offset %" PRIu64 " is not aligned"), path,
> > > range[0]);
> > >  
> > >  	/* is the range end behind the end of the device ?*/
> > >  	if (range[0] > blksize)
> > > @@ -155,6 +155,10 @@ int main(int argc, char **argv)
> > >  	if (end < range[0] || end > blksize)
> > >  		range[1] = blksize - range[0];
> > >  
> > > +	/* check length alignment to the sector size */
> > > +	if (range[1] % secsize)
> > > +		errx(EXIT_FAILURE, _("%s: length %" PRIu64 " is not aligned"), path,
> > > range[1]);
> > > +
> > 
> > Looks good, however remember that the tool is intended to be used by
> > the end user, can we be a bit more helpful ? We already know the
> > secsize it should be aligned to, so we can print that as well.
> 
> I will update the error message.
> 
> > However it just occurred to me that aligning it to secsize is not
> > enough. It should be aligned to discard_granularity.
> 
> Aligning to discard_granularity is an optimization, on the other hand
> not aligning to secsize would result in the ioctl failing (EINVAL).

Fair enough, but the unaligned part will be ignored and not
'discarded'. But I guess that it really does not matter.

-Lukas

> 
>