| Message ID | 20170212014724.10618-1-mreitz@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Sun, 02/12 02:47, Max Reitz wrote: > Defining BDRV_REQUEST_MAX_SECTORS based on BDRV_REQUEST_MAX_BYTES is > simpler than the other way around. > > Signed-off-by: Max Reitz <mreitz@redhat.com> > --- > include/block/block.h | 5 ++--- > 1 file changed, 2 insertions(+), 3 deletions(-) > > diff --git a/include/block/block.h b/include/block/block.h > index 4e81f2069b..101ef33f6b 100644 > --- a/include/block/block.h > +++ b/include/block/block.h > @@ -114,9 +114,8 @@ typedef struct HDGeometry { > #define BDRV_SECTOR_SIZE (1ULL << BDRV_SECTOR_BITS) > #define BDRV_SECTOR_MASK ~(BDRV_SECTOR_SIZE - 1) > > -#define BDRV_REQUEST_MAX_SECTORS MIN(SIZE_MAX >> BDRV_SECTOR_BITS, \ > - INT_MAX >> BDRV_SECTOR_BITS) > -#define BDRV_REQUEST_MAX_BYTES (BDRV_REQUEST_MAX_SECTORS << BDRV_SECTOR_BITS) > +#define BDRV_REQUEST_MAX_BYTES MIN(SIZE_MAX, INT_MAX) > +#define BDRV_REQUEST_MAX_SECTORS (BDRV_REQUEST_MAX_BYTES >> BDRV_SECTOR_BITS) > > /* > * Allocation status flags > -- > 2.11.0 > > Reviewed-by: Fam Zheng <famz@redhat.com>
On Sun 12 Feb 2017 02:47:24 AM CET, Max Reitz <mreitz@redhat.com> wrote: > -#define BDRV_REQUEST_MAX_SECTORS MIN(SIZE_MAX >> BDRV_SECTOR_BITS, \ > - INT_MAX >> BDRV_SECTOR_BITS) > -#define BDRV_REQUEST_MAX_BYTES (BDRV_REQUEST_MAX_SECTORS << BDRV_SECTOR_BITS) > +#define BDRV_REQUEST_MAX_BYTES MIN(SIZE_MAX, INT_MAX) > +#define BDRV_REQUEST_MAX_SECTORS (BDRV_REQUEST_MAX_BYTES >> BDRV_SECTOR_BITS) I'm just pointing it out because I don't know if this can cause problems, but this patch would make BDRV_REQUEST_MAX_BYTES not a multiple of the sector size (INT_MAX is actually a prime number). Berto
On 13.02.2017 09:39, Alberto Garcia wrote: > On Sun 12 Feb 2017 02:47:24 AM CET, Max Reitz <mreitz@redhat.com> wrote: > >> -#define BDRV_REQUEST_MAX_SECTORS MIN(SIZE_MAX >> BDRV_SECTOR_BITS, \ >> - INT_MAX >> BDRV_SECTOR_BITS) >> -#define BDRV_REQUEST_MAX_BYTES (BDRV_REQUEST_MAX_SECTORS << BDRV_SECTOR_BITS) >> +#define BDRV_REQUEST_MAX_BYTES MIN(SIZE_MAX, INT_MAX) >> +#define BDRV_REQUEST_MAX_SECTORS (BDRV_REQUEST_MAX_BYTES >> BDRV_SECTOR_BITS) > > I'm just pointing it out because I don't know if this can cause > problems, but this patch would make BDRV_REQUEST_MAX_BYTES not a > multiple of the sector size (INT_MAX is actually a prime number). Very good point. I don't think this could be an issue, though. For one thing, the use of BDRV_REQUEST_MAX_BYTES is very limited. Apart from that, I guess the main issue would be that if you send an unaligned head/tail the automatic round-up would exceed BDRV_REQUEST_MAX_SECTORS. But this is a pre-existing issue, actually: When you issue a request with an unaligned head and 2G - 512 bytes length, bdrv_co_pwritev() will align it to 2G (which is greater than INT_MAX). (Can be tested trivially with: $ qemu-io -c 'write 511 2147483136' --image-opts driver=null-co,size=4G and some debugging code in block/io.c) You can make the request even bigger by increasing the bs->bl.request_alignment. Not a big issue, though, as bdrv_aligned_p{read,write}v() actually take an unsigned int and break down requests greater than INT_MAX automatically. As long as bs->bl.request_alignment stays under 1G or so we'll be fine (i.e. as long as we don't get an unsigned int overflow) -- and when it breaks it won't be because BDRV_REQUEST_MAX_BYTES is not aligned to sectors. Max
On Mon 13 Feb 2017 06:13:38 PM CET, Max Reitz wrote: >>> -#define BDRV_REQUEST_MAX_SECTORS MIN(SIZE_MAX >> BDRV_SECTOR_BITS, \ >>> - INT_MAX >> BDRV_SECTOR_BITS) >>> -#define BDRV_REQUEST_MAX_BYTES (BDRV_REQUEST_MAX_SECTORS << BDRV_SECTOR_BITS) >>> +#define BDRV_REQUEST_MAX_BYTES MIN(SIZE_MAX, INT_MAX) >>> +#define BDRV_REQUEST_MAX_SECTORS (BDRV_REQUEST_MAX_BYTES >> BDRV_SECTOR_BITS) >> >> I'm just pointing it out because I don't know if this can cause >> problems, but this patch would make BDRV_REQUEST_MAX_BYTES not a >> multiple of the sector size (INT_MAX is actually a prime number). > > Very good point. I don't think this could be an issue, though. For one > thing, the use of BDRV_REQUEST_MAX_BYTES is very limited. Ok, but then I wonder what's the benefit of increasing BDRV_REQUEST_MAX_BYTES. Berto
On 14.02.2017 10:52, Alberto Garcia wrote: > On Mon 13 Feb 2017 06:13:38 PM CET, Max Reitz wrote: > >>>> -#define BDRV_REQUEST_MAX_SECTORS MIN(SIZE_MAX >> BDRV_SECTOR_BITS, \ >>>> - INT_MAX >> BDRV_SECTOR_BITS) >>>> -#define BDRV_REQUEST_MAX_BYTES (BDRV_REQUEST_MAX_SECTORS << BDRV_SECTOR_BITS) >>>> +#define BDRV_REQUEST_MAX_BYTES MIN(SIZE_MAX, INT_MAX) >>>> +#define BDRV_REQUEST_MAX_SECTORS (BDRV_REQUEST_MAX_BYTES >> BDRV_SECTOR_BITS) >>> >>> I'm just pointing it out because I don't know if this can cause >>> problems, but this patch would make BDRV_REQUEST_MAX_BYTES not a >>> multiple of the sector size (INT_MAX is actually a prime number). >> >> Very good point. I don't think this could be an issue, though. For one >> thing, the use of BDRV_REQUEST_MAX_BYTES is very limited. > > Ok, but then I wonder what's the benefit of increasing > BDRV_REQUEST_MAX_BYTES. The benefit is that the definition looks cleaner. Max
Am 15.02.2017 um 14:42 hat Max Reitz geschrieben: > On 14.02.2017 10:52, Alberto Garcia wrote: > > On Mon 13 Feb 2017 06:13:38 PM CET, Max Reitz wrote: > > > >>>> -#define BDRV_REQUEST_MAX_SECTORS MIN(SIZE_MAX >> BDRV_SECTOR_BITS, \ > >>>> - INT_MAX >> BDRV_SECTOR_BITS) > >>>> -#define BDRV_REQUEST_MAX_BYTES (BDRV_REQUEST_MAX_SECTORS << BDRV_SECTOR_BITS) > >>>> +#define BDRV_REQUEST_MAX_BYTES MIN(SIZE_MAX, INT_MAX) > >>>> +#define BDRV_REQUEST_MAX_SECTORS (BDRV_REQUEST_MAX_BYTES >> BDRV_SECTOR_BITS) > >>> > >>> I'm just pointing it out because I don't know if this can cause > >>> problems, but this patch would make BDRV_REQUEST_MAX_BYTES not a > >>> multiple of the sector size (INT_MAX is actually a prime number). > >> > >> Very good point. I don't think this could be an issue, though. For one > >> thing, the use of BDRV_REQUEST_MAX_BYTES is very limited. > > > > Ok, but then I wonder what's the benefit of increasing > > BDRV_REQUEST_MAX_BYTES. > > The benefit is that the definition looks cleaner. Whatever way we want to write it, I think MAX_BYTES = MAX_SECTORS * 512 should be a given. Everything else is bound to confuse people and introduce bugs sooner or later. Kevin
On 15.02.2017 17:44, Kevin Wolf wrote: > Am 15.02.2017 um 14:42 hat Max Reitz geschrieben: >> On 14.02.2017 10:52, Alberto Garcia wrote: >>> On Mon 13 Feb 2017 06:13:38 PM CET, Max Reitz wrote: >>> >>>>>> -#define BDRV_REQUEST_MAX_SECTORS MIN(SIZE_MAX >> BDRV_SECTOR_BITS, \ >>>>>> - INT_MAX >> BDRV_SECTOR_BITS) >>>>>> -#define BDRV_REQUEST_MAX_BYTES (BDRV_REQUEST_MAX_SECTORS << BDRV_SECTOR_BITS) >>>>>> +#define BDRV_REQUEST_MAX_BYTES MIN(SIZE_MAX, INT_MAX) >>>>>> +#define BDRV_REQUEST_MAX_SECTORS (BDRV_REQUEST_MAX_BYTES >> BDRV_SECTOR_BITS) >>>>> >>>>> I'm just pointing it out because I don't know if this can cause >>>>> problems, but this patch would make BDRV_REQUEST_MAX_BYTES not a >>>>> multiple of the sector size (INT_MAX is actually a prime number). >>>> >>>> Very good point. I don't think this could be an issue, though. For one >>>> thing, the use of BDRV_REQUEST_MAX_BYTES is very limited. >>> >>> Ok, but then I wonder what's the benefit of increasing >>> BDRV_REQUEST_MAX_BYTES. >> >> The benefit is that the definition looks cleaner. > > Whatever way we want to write it, I think MAX_BYTES = MAX_SECTORS * 512 > should be a given. Everything else is bound to confuse people and > introduce bugs sooner or later. Probably only sooner and not later, considering we are switching to byte granularity overall anyway. And if something confuses people, I'd argue it's the fact that we still have sector granularity all over the place and not that your requests can be a bit bigger if you submit them in bytes than if you submit them in sectors. Anyway, if MAX_BYTES should be a multiple of the sector size, then I can't think of a much better way to write this than what we currently have and this patch is unneeded. Max
Am 15.02.2017 um 17:48 hat Max Reitz geschrieben: > On 15.02.2017 17:44, Kevin Wolf wrote: > > Am 15.02.2017 um 14:42 hat Max Reitz geschrieben: > >> On 14.02.2017 10:52, Alberto Garcia wrote: > >>> On Mon 13 Feb 2017 06:13:38 PM CET, Max Reitz wrote: > >>> > >>>>>> -#define BDRV_REQUEST_MAX_SECTORS MIN(SIZE_MAX >> BDRV_SECTOR_BITS, \ > >>>>>> - INT_MAX >> BDRV_SECTOR_BITS) > >>>>>> -#define BDRV_REQUEST_MAX_BYTES (BDRV_REQUEST_MAX_SECTORS << BDRV_SECTOR_BITS) > >>>>>> +#define BDRV_REQUEST_MAX_BYTES MIN(SIZE_MAX, INT_MAX) > >>>>>> +#define BDRV_REQUEST_MAX_SECTORS (BDRV_REQUEST_MAX_BYTES >> BDRV_SECTOR_BITS) > >>>>> > >>>>> I'm just pointing it out because I don't know if this can cause > >>>>> problems, but this patch would make BDRV_REQUEST_MAX_BYTES not a > >>>>> multiple of the sector size (INT_MAX is actually a prime number). > >>>> > >>>> Very good point. I don't think this could be an issue, though. For one > >>>> thing, the use of BDRV_REQUEST_MAX_BYTES is very limited. > >>> > >>> Ok, but then I wonder what's the benefit of increasing > >>> BDRV_REQUEST_MAX_BYTES. > >> > >> The benefit is that the definition looks cleaner. > > > > Whatever way we want to write it, I think MAX_BYTES = MAX_SECTORS * 512 > > should be a given. Everything else is bound to confuse people and > > introduce bugs sooner or later. > > Probably only sooner and not later, considering we are switching to byte > granularity overall anyway. And if something confuses people, I'd argue > it's the fact that we still have sector granularity all over the place > and not that your requests can be a bit bigger if you submit them in > bytes than if you submit them in sectors. > > Anyway, if MAX_BYTES should be a multiple of the sector size, then I > can't think of a much better way to write this than what we currently > have and this patch is unneeded. Maybe we can just get rid of BDRV_REQUEST_MAX_SECTORS? Or do we need to do a few more conversion before that? Kevin
On 15.02.2017 18:10, Kevin Wolf wrote: > Am 15.02.2017 um 17:48 hat Max Reitz geschrieben: >> On 15.02.2017 17:44, Kevin Wolf wrote: >>> Am 15.02.2017 um 14:42 hat Max Reitz geschrieben: >>>> On 14.02.2017 10:52, Alberto Garcia wrote: >>>>> On Mon 13 Feb 2017 06:13:38 PM CET, Max Reitz wrote: >>>>> >>>>>>>> -#define BDRV_REQUEST_MAX_SECTORS MIN(SIZE_MAX >> BDRV_SECTOR_BITS, \ >>>>>>>> - INT_MAX >> BDRV_SECTOR_BITS) >>>>>>>> -#define BDRV_REQUEST_MAX_BYTES (BDRV_REQUEST_MAX_SECTORS << BDRV_SECTOR_BITS) >>>>>>>> +#define BDRV_REQUEST_MAX_BYTES MIN(SIZE_MAX, INT_MAX) >>>>>>>> +#define BDRV_REQUEST_MAX_SECTORS (BDRV_REQUEST_MAX_BYTES >> BDRV_SECTOR_BITS) >>>>>>> >>>>>>> I'm just pointing it out because I don't know if this can cause >>>>>>> problems, but this patch would make BDRV_REQUEST_MAX_BYTES not a >>>>>>> multiple of the sector size (INT_MAX is actually a prime number). >>>>>> >>>>>> Very good point. I don't think this could be an issue, though. For one >>>>>> thing, the use of BDRV_REQUEST_MAX_BYTES is very limited. >>>>> >>>>> Ok, but then I wonder what's the benefit of increasing >>>>> BDRV_REQUEST_MAX_BYTES. >>>> >>>> The benefit is that the definition looks cleaner. >>> >>> Whatever way we want to write it, I think MAX_BYTES = MAX_SECTORS * 512 >>> should be a given. Everything else is bound to confuse people and >>> introduce bugs sooner or later. >> >> Probably only sooner and not later, considering we are switching to byte >> granularity overall anyway. And if something confuses people, I'd argue >> it's the fact that we still have sector granularity all over the place >> and not that your requests can be a bit bigger if you submit them in >> bytes than if you submit them in sectors. >> >> Anyway, if MAX_BYTES should be a multiple of the sector size, then I >> can't think of a much better way to write this than what we currently >> have and this patch is unneeded. > > Maybe we can just get rid of BDRV_REQUEST_MAX_SECTORS? Or do we need to > do a few more conversion before that? We probably could, but it wouldn't make much sense. We have many places that use BDRV_REQUEST_MAX_SECTORS without multiplying it by 512 and if we decided to use *_MAX_BYTES dividing it by 512 in every one of those places, we could just as well define a central macro for that -- which would be *_MAX_SECTORS, so... Max
diff --git a/include/block/block.h b/include/block/block.h index 4e81f2069b..101ef33f6b 100644 --- a/include/block/block.h +++ b/include/block/block.h @@ -114,9 +114,8 @@ typedef struct HDGeometry { #define BDRV_SECTOR_SIZE (1ULL << BDRV_SECTOR_BITS) #define BDRV_SECTOR_MASK ~(BDRV_SECTOR_SIZE - 1) -#define BDRV_REQUEST_MAX_SECTORS MIN(SIZE_MAX >> BDRV_SECTOR_BITS, \ - INT_MAX >> BDRV_SECTOR_BITS) -#define BDRV_REQUEST_MAX_BYTES (BDRV_REQUEST_MAX_SECTORS << BDRV_SECTOR_BITS) +#define BDRV_REQUEST_MAX_BYTES MIN(SIZE_MAX, INT_MAX) +#define BDRV_REQUEST_MAX_SECTORS (BDRV_REQUEST_MAX_BYTES >> BDRV_SECTOR_BITS) /* * Allocation status flags
Defining BDRV_REQUEST_MAX_SECTORS based on BDRV_REQUEST_MAX_BYTES is simpler than the other way around. Signed-off-by: Max Reitz <mreitz@redhat.com> --- include/block/block.h | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-)