On Fri, 24 Oct 2014, Federico Simoncelli wrote:
> Date: Fri, 24 Oct 2014 05:47:04 -0400 (EDT)
> From: Federico Simoncelli <fsimonce@xxxxxxxxxx>
> To: Lukáš Czerner <lczerner@xxxxxxxxxx>
> Cc: util-linux@xxxxxxxxxxxxxxx, kzak@xxxxxxxxxx
> Subject: Re: [PATCH 2/2] blkdiscard: fail on sector misalignment
>
> ----- Original Message -----
> > From: "Lukáš Czerner" <lczerner@xxxxxxxxxx>
> > To: "Federico Simoncelli" <fsimonce@xxxxxxxxxx>
> > Cc: util-linux@xxxxxxxxxxxxxxx, kzak@xxxxxxxxxx
> > 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@xxxxxxxxxx>
> > > To: util-linux@xxxxxxxxxxxxxxx
> > > Cc: kzak@xxxxxxxxxx, lczerner@xxxxxxxxxx,
> > > Federico Simoncelli <fsimonce@xxxxxxxxxx>
> > > 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
>
>
