| Message ID | 1416392276-10408-3-git-send-email-tumanova@linux.vnet.ibm.com |
|---|---|
| State | New |
| Headers | show |
Am 19.11.2014 um 11:17 schrieb Ekaterina Tumanova: > Move the IOCTL calls that detect logical blocksize from raw_probe_alignment > into separate function (probe_logical_blocksize). > Introduce function which detect physical blocksize via IOCTL > (probe_physical_blocksize). > Both functions will be used in the next patch. > > Signed-off-by: Ekaterina Tumanova <tumanova@linux.vnet.ibm.com> From what I can tell this should be a no-op for raw_probe_alignment. probe_physical_blocksize looks also good. When this patch is applied stand-alone, gcc will complain about a defined but unused function, though. So we might want to move this function into patch 3 or just add an __attribute__((unused)) here (and remove that in patch 3). Or just leave it as is. Otherwise Reviewed-by: Christian Borntraeger <borntraeger@de.ibm.com> > --- > block/raw-posix.c | 58 +++++++++++++++++++++++++++++++++++++------------------ > 1 file changed, 39 insertions(+), 19 deletions(-) > > diff --git a/block/raw-posix.c b/block/raw-posix.c > index e100ae2..45f1d79 100644 > --- a/block/raw-posix.c > +++ b/block/raw-posix.c > @@ -223,50 +223,70 @@ static int raw_normalize_devicepath(const char **filename) > } > #endif > > -static void raw_probe_alignment(BlockDriverState *bs, int fd, Error **errp) > +static unsigned int probe_logical_blocksize(BlockDriverState *bs, int fd) > { > - BDRVRawState *s = bs->opaque; > - char *buf; > - unsigned int sector_size; > - > - /* For /dev/sg devices the alignment is not really used. > - With buffered I/O, we don't have any restrictions. */ > - if (bs->sg || !s->needs_alignment) { > - bs->request_alignment = 1; > - s->buf_align = 1; > - return; > - } > + unsigned int sector_size = 0; > > /* Try a few ioctls to get the right size */ > - bs->request_alignment = 0; > - s->buf_align = 0; > - > #ifdef BLKSSZGET > if (ioctl(fd, BLKSSZGET, §or_size) >= 0) { > - bs->request_alignment = sector_size; > + return sector_size; > } > #endif > #ifdef DKIOCGETBLOCKSIZE > if (ioctl(fd, DKIOCGETBLOCKSIZE, §or_size) >= 0) { > - bs->request_alignment = sector_size; > + return sector_size; > } > #endif > #ifdef DIOCGSECTORSIZE > if (ioctl(fd, DIOCGSECTORSIZE, §or_size) >= 0) { > - bs->request_alignment = sector_size; > + return sector_size; > } > #endif > #ifdef CONFIG_XFS > if (s->is_xfs) { > struct dioattr da; > if (xfsctl(NULL, fd, XFS_IOC_DIOINFO, &da) >= 0) { > - bs->request_alignment = da.d_miniosz; > + sector_size = da.d_miniosz; > /* The kernel returns wrong information for d_mem */ > /* s->buf_align = da.d_mem; */ > + return sector_size; > } > } > #endif > > + return 0; > +} > + > +static unsigned int probe_physical_blocksize(BlockDriverState *bs, int fd) > +{ > + unsigned int blk_size = 0; > +#ifdef BLKPBSZGET > + if (ioctl(fd, BLKPBSZGET, &blk_size) >= 0) { > + return blk_size; > + } > +#endif > + > + return 0; > +} > + > +static void raw_probe_alignment(BlockDriverState *bs, int fd, Error **errp) > +{ > + BDRVRawState *s = bs->opaque; > + char *buf; > + > + /* For /dev/sg devices the alignment is not really used. > + With buffered I/O, we don't have any restrictions. */ > + if (bs->sg || !s->needs_alignment) { > + bs->request_alignment = 1; > + s->buf_align = 1; > + return; > + } > + > + s->buf_align = 0; > + /* Let's try to use the logical blocksize for the alignment. */ > + bs->request_alignment = probe_logical_blocksize(bs, fd); > + > /* If we could not get the sizes so far, we can only guess them */ > if (!s->buf_align) { > size_t align; >
On Fri, Nov 21, 2014 at 11:17:02AM +0100, Christian Borntraeger wrote: > Am 19.11.2014 um 11:17 schrieb Ekaterina Tumanova: > > Move the IOCTL calls that detect logical blocksize from raw_probe_alignment > > into separate function (probe_logical_blocksize). > > Introduce function which detect physical blocksize via IOCTL > > (probe_physical_blocksize). > > Both functions will be used in the next patch. > > > > Signed-off-by: Ekaterina Tumanova <tumanova@linux.vnet.ibm.com> > > From what I can tell this should be a no-op for raw_probe_alignment. > > probe_physical_blocksize looks also good. When this patch is applied stand-alone, > gcc will complain about a defined but unused function, though. > > So we might want to move this function into patch 3 or just add an __attribute__((unused)) > here (and remove that in patch 3). Or just leave it as is. Please move probe_physical_blocksize() to Patch 3 because some QEMU builds default to -Werror where this patch causes a build failure. (In order for git-bisect(1) to work patches must not break the build.) Stefan
Ekaterina Tumanova <tumanova@linux.vnet.ibm.com> writes: > Move the IOCTL calls that detect logical blocksize from raw_probe_alignment > into separate function (probe_logical_blocksize). > Introduce function which detect physical blocksize via IOCTL > (probe_physical_blocksize). > Both functions will be used in the next patch. The first one is used in this patch, actually. > > Signed-off-by: Ekaterina Tumanova <tumanova@linux.vnet.ibm.com> Subject's subsystem is "geometry". Geometry is your topic. The subsystem is what you're patching. Here, it's "raw-posix" or "block/raw-posix". Likewise for the other patches in this series. Stefan asked you move probe_physical_blocksize() to the patch that uses it, and I concur. With that done, I'd write a commit message like raw-posix: Factor block size detection out of raw_probe_alignment() Put it in new probe_logical_blocksize(). > --- > block/raw-posix.c | 58 +++++++++++++++++++++++++++++++++++++------------------ > 1 file changed, 39 insertions(+), 19 deletions(-) > > diff --git a/block/raw-posix.c b/block/raw-posix.c > index e100ae2..45f1d79 100644 > --- a/block/raw-posix.c > +++ b/block/raw-posix.c > @@ -223,50 +223,70 @@ static int raw_normalize_devicepath(const char **filename) > } > #endif > > -static void raw_probe_alignment(BlockDriverState *bs, int fd, Error **errp) > +static unsigned int probe_logical_blocksize(BlockDriverState *bs, int fd) Parameter bs is unused, let's drop it. Suggest function comment /** * Return logical block size, or zero if we can't figure it out */ > { > - BDRVRawState *s = bs->opaque; > - char *buf; > - unsigned int sector_size; > - > - /* For /dev/sg devices the alignment is not really used. > - With buffered I/O, we don't have any restrictions. */ > - if (bs->sg || !s->needs_alignment) { > - bs->request_alignment = 1; > - s->buf_align = 1; > - return; > - } > + unsigned int sector_size = 0; Pointless initialization. > > /* Try a few ioctls to get the right size */ > - bs->request_alignment = 0; > - s->buf_align = 0; > - > #ifdef BLKSSZGET > if (ioctl(fd, BLKSSZGET, §or_size) >= 0) { > - bs->request_alignment = sector_size; > + return sector_size; > } > #endif > #ifdef DKIOCGETBLOCKSIZE > if (ioctl(fd, DKIOCGETBLOCKSIZE, §or_size) >= 0) { > - bs->request_alignment = sector_size; > + return sector_size; > } > #endif > #ifdef DIOCGSECTORSIZE > if (ioctl(fd, DIOCGSECTORSIZE, §or_size) >= 0) { > - bs->request_alignment = sector_size; > + return sector_size; > } > #endif > #ifdef CONFIG_XFS > if (s->is_xfs) { > struct dioattr da; > if (xfsctl(NULL, fd, XFS_IOC_DIOINFO, &da) >= 0) { > - bs->request_alignment = da.d_miniosz; > + sector_size = da.d_miniosz; > /* The kernel returns wrong information for d_mem */ > /* s->buf_align = da.d_mem; */ Since you keep the enabled assignments to s->buf_align out of this function, you should keep out this disabled one, too. > + return sector_size; > } > } > #endif > > + return 0; > +} > + > +static unsigned int probe_physical_blocksize(BlockDriverState *bs, int fd) Parameter bs is unused, let's drop it. > +{ > + unsigned int blk_size = 0; Pointless initialization. > +#ifdef BLKPBSZGET > + if (ioctl(fd, BLKPBSZGET, &blk_size) >= 0) { > + return blk_size; > + } > +#endif > + > + return 0; > +} > + > +static void raw_probe_alignment(BlockDriverState *bs, int fd, Error **errp) > +{ > + BDRVRawState *s = bs->opaque; > + char *buf; > + > + /* For /dev/sg devices the alignment is not really used. > + With buffered I/O, we don't have any restrictions. */ > + if (bs->sg || !s->needs_alignment) { > + bs->request_alignment = 1; > + s->buf_align = 1; > + return; > + } > + > + s->buf_align = 0; > + /* Let's try to use the logical blocksize for the alignment. */ > + bs->request_alignment = probe_logical_blocksize(bs, fd); > + > /* If we could not get the sizes so far, we can only guess them */ > if (!s->buf_align) { > size_t align;
> > Suggest function comment > > /** > * Return logical block size, or zero if we can't figure it out > */ > >> { >> - BDRVRawState *s = bs->opaque; >> - char *buf; >> - unsigned int sector_size; >> - >> - /* For /dev/sg devices the alignment is not really used. >> - With buffered I/O, we don't have any restrictions. */ >> - if (bs->sg || !s->needs_alignment) { >> - bs->request_alignment = 1; >> - s->buf_align = 1; >> - return; >> - } >> + unsigned int sector_size = 0; > > Pointless initialization. If I do not initialize the sector_size, and the ioctl fails, I will return garbage as a blocksize to the caller. > >> >> /* Try a few ioctls to get the right size */ >> - bs->request_alignment = 0; >> - s->buf_align = 0; >> - >> #ifdef BLKSSZGET >> if (ioctl(fd, BLKSSZGET, §or_size) >= 0) { >> - bs->request_alignment = sector_size; >> + return sector_size; >> } >> #endif >> #ifdef DKIOCGETBLOCKSIZE >> if (ioctl(fd, DKIOCGETBLOCKSIZE, §or_size) >= 0) { >> - bs->request_alignment = sector_size; >> + return sector_size; >> } >> #endif >> #ifdef DIOCGSECTORSIZE >> if (ioctl(fd, DIOCGSECTORSIZE, §or_size) >= 0) { >> - bs->request_alignment = sector_size; >> + return sector_size; >> } >> #endif >> #ifdef CONFIG_XFS >> if (s->is_xfs) { >> struct dioattr da; >> if (xfsctl(NULL, fd, XFS_IOC_DIOINFO, &da) >= 0) { >> - bs->request_alignment = da.d_miniosz; >> + sector_size = da.d_miniosz; >> /* The kernel returns wrong information for d_mem */ >> /* s->buf_align = da.d_mem; */ > > Since you keep the enabled assignments to s->buf_align out of this > function, you should keep out this disabled one, too. > >> + return sector_size; >> } >> } >> #endif >> >> + return 0; >> +} >> + >> +static unsigned int probe_physical_blocksize(BlockDriverState *bs, int fd) > > Parameter bs is unused, let's drop it. > >> +{ >> + unsigned int blk_size = 0; > > Pointless initialization. Same here. > >> +#ifdef BLKPBSZGET >> + if (ioctl(fd, BLKPBSZGET, &blk_size) >= 0) { >> + return blk_size; >> + } >> +#endif >> + >> + return 0; >> +} >> + >> +static void raw_probe_alignment(BlockDriverState *bs, int fd, Error **errp) >> +{ >> + BDRVRawState *s = bs->opaque; >> + char *buf; >> + >> + /* For /dev/sg devices the alignment is not really used. >> + With buffered I/O, we don't have any restrictions. */ >> + if (bs->sg || !s->needs_alignment) { >> + bs->request_alignment = 1; >> + s->buf_align = 1; >> + return; >> + } >> + >> + s->buf_align = 0; >> + /* Let's try to use the logical blocksize for the alignment. */ >> + bs->request_alignment = probe_logical_blocksize(bs, fd); >> + >> /* If we could not get the sizes so far, we can only guess them */ >> if (!s->buf_align) { >> size_t align; >
Ekaterina Tumanova <tumanova@linux.vnet.ibm.com> writes: >> >> Suggest function comment >> >> /** >> * Return logical block size, or zero if we can't figure it out >> */ >> >>> { >>> - BDRVRawState *s = bs->opaque; >>> - char *buf; >>> - unsigned int sector_size; >>> - >>> - /* For /dev/sg devices the alignment is not really used. >>> - With buffered I/O, we don't have any restrictions. */ >>> - if (bs->sg || !s->needs_alignment) { >>> - bs->request_alignment = 1; >>> - s->buf_align = 1; >>> - return; >>> - } >>> + unsigned int sector_size = 0; >> >> Pointless initialization. > > If I do not initialize the sector_size, and the ioctl fails, > I will return garbage as a blocksize to the caller. Where? As far as I can see, we return it only after ioctl() succeeded. >>> >>> /* Try a few ioctls to get the right size */ >>> - bs->request_alignment = 0; >>> - s->buf_align = 0; >>> - >>> #ifdef BLKSSZGET >>> if (ioctl(fd, BLKSSZGET, §or_size) >= 0) { >>> - bs->request_alignment = sector_size; >>> + return sector_size; >>> } >>> #endif >>> #ifdef DKIOCGETBLOCKSIZE >>> if (ioctl(fd, DKIOCGETBLOCKSIZE, §or_size) >= 0) { >>> - bs->request_alignment = sector_size; >>> + return sector_size; >>> } >>> #endif >>> #ifdef DIOCGSECTORSIZE >>> if (ioctl(fd, DIOCGSECTORSIZE, §or_size) >= 0) { >>> - bs->request_alignment = sector_size; >>> + return sector_size; >>> } >>> #endif >>> #ifdef CONFIG_XFS >>> if (s->is_xfs) { >>> struct dioattr da; >>> if (xfsctl(NULL, fd, XFS_IOC_DIOINFO, &da) >= 0) { >>> - bs->request_alignment = da.d_miniosz; >>> + sector_size = da.d_miniosz; >>> /* The kernel returns wrong information for d_mem */ >>> /* s->buf_align = da.d_mem; */ >> >> Since you keep the enabled assignments to s->buf_align out of this >> function, you should keep out this disabled one, too. >> >>> + return sector_size; >>> } >>> } >>> #endif >>> >>> + return 0; >>> +} >>> + >>> +static unsigned int probe_physical_blocksize(BlockDriverState *bs, int fd) >> >> Parameter bs is unused, let's drop it. >> >>> +{ >>> + unsigned int blk_size = 0; >> >> Pointless initialization. > > Same here. Again, we return it only after ioctl() succeeded. >>> +#ifdef BLKPBSZGET >>> + if (ioctl(fd, BLKPBSZGET, &blk_size) >= 0) { >>> + return blk_size; >>> + } >>> +#endif >>> + >>> + return 0; >>> +} >>> + >>> +static void raw_probe_alignment(BlockDriverState *bs, int fd, Error **errp) >>> +{ >>> + BDRVRawState *s = bs->opaque; >>> + char *buf; >>> + >>> + /* For /dev/sg devices the alignment is not really used. >>> + With buffered I/O, we don't have any restrictions. */ >>> + if (bs->sg || !s->needs_alignment) { >>> + bs->request_alignment = 1; >>> + s->buf_align = 1; >>> + return; >>> + } >>> + >>> + s->buf_align = 0; >>> + /* Let's try to use the logical blocksize for the alignment. */ >>> + bs->request_alignment = probe_logical_blocksize(bs, fd); >>> + >>> /* If we could not get the sizes so far, we can only guess them */ >>> if (!s->buf_align) { >>> size_t align; >>
On 11/28/2014 03:52 PM, Markus Armbruster wrote: > Ekaterina Tumanova <tumanova@linux.vnet.ibm.com> writes: > >>> >>> Suggest function comment >>> >>> /** >>> * Return logical block size, or zero if we can't figure it out >>> */ >>> >>>> { >>>> - BDRVRawState *s = bs->opaque; >>>> - char *buf; >>>> - unsigned int sector_size; >>>> - >>>> - /* For /dev/sg devices the alignment is not really used. >>>> - With buffered I/O, we don't have any restrictions. */ >>>> - if (bs->sg || !s->needs_alignment) { >>>> - bs->request_alignment = 1; >>>> - s->buf_align = 1; >>>> - return; >>>> - } >>>> + unsigned int sector_size = 0; >>> >>> Pointless initialization. >> >> If I do not initialize the sector_size, and the ioctl fails, >> I will return garbage as a blocksize to the caller. > > Where? As far as I can see, we return it only after ioctl() succeeded. > Sorry, you're absolutely right. I kept seeing and thinking that I always returned sector_size variable. ::facepalm:: >>>> >>>> /* Try a few ioctls to get the right size */ >>>> - bs->request_alignment = 0; >>>> - s->buf_align = 0; >>>> - >>>> #ifdef BLKSSZGET >>>> if (ioctl(fd, BLKSSZGET, §or_size) >= 0) { >>>> - bs->request_alignment = sector_size; >>>> + return sector_size; >>>> } >>>> #endif >>>> #ifdef DKIOCGETBLOCKSIZE >>>> if (ioctl(fd, DKIOCGETBLOCKSIZE, §or_size) >= 0) { >>>> - bs->request_alignment = sector_size; >>>> + return sector_size; >>>> } >>>> #endif >>>> #ifdef DIOCGSECTORSIZE >>>> if (ioctl(fd, DIOCGSECTORSIZE, §or_size) >= 0) { >>>> - bs->request_alignment = sector_size; >>>> + return sector_size; >>>> } >>>> #endif >>>> #ifdef CONFIG_XFS >>>> if (s->is_xfs) { >>>> struct dioattr da; >>>> if (xfsctl(NULL, fd, XFS_IOC_DIOINFO, &da) >= 0) { >>>> - bs->request_alignment = da.d_miniosz; >>>> + sector_size = da.d_miniosz; >>>> /* The kernel returns wrong information for d_mem */ >>>> /* s->buf_align = da.d_mem; */ >>> >>> Since you keep the enabled assignments to s->buf_align out of this >>> function, you should keep out this disabled one, too. >>> >>>> + return sector_size; >>>> } >>>> } >>>> #endif >>>> >>>> + return 0; >>>> +} >>>> + >>>> +static unsigned int probe_physical_blocksize(BlockDriverState *bs, int fd) >>> >>> Parameter bs is unused, let's drop it. >>> >>>> +{ >>>> + unsigned int blk_size = 0; >>> >>> Pointless initialization. >> >> Same here. > > Again, we return it only after ioctl() succeeded. > >>>> +#ifdef BLKPBSZGET >>>> + if (ioctl(fd, BLKPBSZGET, &blk_size) >= 0) { >>>> + return blk_size; >>>> + } >>>> +#endif >>>> + >>>> + return 0; >>>> +} >>>> + >>>> +static void raw_probe_alignment(BlockDriverState *bs, int fd, Error **errp) >>>> +{ >>>> + BDRVRawState *s = bs->opaque; >>>> + char *buf; >>>> + >>>> + /* For /dev/sg devices the alignment is not really used. >>>> + With buffered I/O, we don't have any restrictions. */ >>>> + if (bs->sg || !s->needs_alignment) { >>>> + bs->request_alignment = 1; >>>> + s->buf_align = 1; >>>> + return; >>>> + } >>>> + >>>> + s->buf_align = 0; >>>> + /* Let's try to use the logical blocksize for the alignment. */ >>>> + bs->request_alignment = probe_logical_blocksize(bs, fd); >>>> + >>>> /* If we could not get the sizes so far, we can only guess them */ >>>> if (!s->buf_align) { >>>> size_t align; >>> >
diff --git a/block/raw-posix.c b/block/raw-posix.c index e100ae2..45f1d79 100644 --- a/block/raw-posix.c +++ b/block/raw-posix.c @@ -223,50 +223,70 @@ static int raw_normalize_devicepath(const char **filename) } #endif -static void raw_probe_alignment(BlockDriverState *bs, int fd, Error **errp) +static unsigned int probe_logical_blocksize(BlockDriverState *bs, int fd) { - BDRVRawState *s = bs->opaque; - char *buf; - unsigned int sector_size; - - /* For /dev/sg devices the alignment is not really used. - With buffered I/O, we don't have any restrictions. */ - if (bs->sg || !s->needs_alignment) { - bs->request_alignment = 1; - s->buf_align = 1; - return; - } + unsigned int sector_size = 0; /* Try a few ioctls to get the right size */ - bs->request_alignment = 0; - s->buf_align = 0; - #ifdef BLKSSZGET if (ioctl(fd, BLKSSZGET, §or_size) >= 0) { - bs->request_alignment = sector_size; + return sector_size; } #endif #ifdef DKIOCGETBLOCKSIZE if (ioctl(fd, DKIOCGETBLOCKSIZE, §or_size) >= 0) { - bs->request_alignment = sector_size; + return sector_size; } #endif #ifdef DIOCGSECTORSIZE if (ioctl(fd, DIOCGSECTORSIZE, §or_size) >= 0) { - bs->request_alignment = sector_size; + return sector_size; } #endif #ifdef CONFIG_XFS if (s->is_xfs) { struct dioattr da; if (xfsctl(NULL, fd, XFS_IOC_DIOINFO, &da) >= 0) { - bs->request_alignment = da.d_miniosz; + sector_size = da.d_miniosz; /* The kernel returns wrong information for d_mem */ /* s->buf_align = da.d_mem; */ + return sector_size; } } #endif + return 0; +} + +static unsigned int probe_physical_blocksize(BlockDriverState *bs, int fd) +{ + unsigned int blk_size = 0; +#ifdef BLKPBSZGET + if (ioctl(fd, BLKPBSZGET, &blk_size) >= 0) { + return blk_size; + } +#endif + + return 0; +} + +static void raw_probe_alignment(BlockDriverState *bs, int fd, Error **errp) +{ + BDRVRawState *s = bs->opaque; + char *buf; + + /* For /dev/sg devices the alignment is not really used. + With buffered I/O, we don't have any restrictions. */ + if (bs->sg || !s->needs_alignment) { + bs->request_alignment = 1; + s->buf_align = 1; + return; + } + + s->buf_align = 0; + /* Let's try to use the logical blocksize for the alignment. */ + bs->request_alignment = probe_logical_blocksize(bs, fd); + /* If we could not get the sizes so far, we can only guess them */ if (!s->buf_align) { size_t align;
Move the IOCTL calls that detect logical blocksize from raw_probe_alignment into separate function (probe_logical_blocksize). Introduce function which detect physical blocksize via IOCTL (probe_physical_blocksize). Both functions will be used in the next patch. Signed-off-by: Ekaterina Tumanova <tumanova@linux.vnet.ibm.com> --- block/raw-posix.c | 58 +++++++++++++++++++++++++++++++++++++------------------ 1 file changed, 39 insertions(+), 19 deletions(-)