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

Federico Simoncelli
2014-10-24T09:47:06Z

----- 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).

--
Federico