* [Qemu-devel] [PATCH] block: Swap request limit definitions @ 2017-02-12 1:47 Max Reitz 2017-02-13 5:52 ` Fam Zheng 2017-02-13 8:39 ` [Qemu-devel] [Qemu-block] " Alberto Garcia 0 siblings, 2 replies; 10+ messages in thread From: Max Reitz @ 2017-02-12 1:47 UTC (permalink / raw) To: qemu-block; +Cc: qemu-devel, Max Reitz, Kevin Wolf 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 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [Qemu-devel] [PATCH] block: Swap request limit definitions 2017-02-12 1:47 [Qemu-devel] [PATCH] block: Swap request limit definitions Max Reitz @ 2017-02-13 5:52 ` Fam Zheng 2017-02-13 8:39 ` [Qemu-devel] [Qemu-block] " Alberto Garcia 1 sibling, 0 replies; 10+ messages in thread From: Fam Zheng @ 2017-02-13 5:52 UTC (permalink / raw) To: Max Reitz; +Cc: qemu-block, Kevin Wolf, qemu-devel 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> ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [Qemu-devel] [Qemu-block] [PATCH] block: Swap request limit definitions 2017-02-12 1:47 [Qemu-devel] [PATCH] block: Swap request limit definitions Max Reitz 2017-02-13 5:52 ` Fam Zheng @ 2017-02-13 8:39 ` Alberto Garcia 2017-02-13 17:13 ` Max Reitz 1 sibling, 1 reply; 10+ messages in thread From: Alberto Garcia @ 2017-02-13 8:39 UTC (permalink / raw) To: Max Reitz, qemu-block; +Cc: Kevin Wolf, qemu-devel 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [Qemu-devel] [Qemu-block] [PATCH] block: Swap request limit definitions 2017-02-13 8:39 ` [Qemu-devel] [Qemu-block] " Alberto Garcia @ 2017-02-13 17:13 ` Max Reitz 2017-02-14 9:52 ` Alberto Garcia 0 siblings, 1 reply; 10+ messages in thread From: Max Reitz @ 2017-02-13 17:13 UTC (permalink / raw) To: Alberto Garcia, qemu-block; +Cc: Kevin Wolf, qemu-devel [-- Attachment #1: Type: text/plain, Size: 1812 bytes --] 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 [-- Attachment #2: OpenPGP digital signature --] [-- Type: application/pgp-signature, Size: 512 bytes --] ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [Qemu-devel] [Qemu-block] [PATCH] block: Swap request limit definitions 2017-02-13 17:13 ` Max Reitz @ 2017-02-14 9:52 ` Alberto Garcia 2017-02-15 13:42 ` Max Reitz 0 siblings, 1 reply; 10+ messages in thread From: Alberto Garcia @ 2017-02-14 9:52 UTC (permalink / raw) To: Max Reitz, qemu-block; +Cc: Kevin Wolf, qemu-devel 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [Qemu-devel] [Qemu-block] [PATCH] block: Swap request limit definitions 2017-02-14 9:52 ` Alberto Garcia @ 2017-02-15 13:42 ` Max Reitz 2017-02-15 16:44 ` Kevin Wolf 0 siblings, 1 reply; 10+ messages in thread From: Max Reitz @ 2017-02-15 13:42 UTC (permalink / raw) To: Alberto Garcia, qemu-block; +Cc: Kevin Wolf, qemu-devel [-- Attachment #1: Type: text/plain, Size: 999 bytes --] 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 [-- Attachment #2: OpenPGP digital signature --] [-- Type: application/pgp-signature, Size: 512 bytes --] ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [Qemu-devel] [Qemu-block] [PATCH] block: Swap request limit definitions 2017-02-15 13:42 ` Max Reitz @ 2017-02-15 16:44 ` Kevin Wolf 2017-02-15 16:48 ` Max Reitz 0 siblings, 1 reply; 10+ messages in thread From: Kevin Wolf @ 2017-02-15 16:44 UTC (permalink / raw) To: Max Reitz; +Cc: Alberto Garcia, qemu-block, qemu-devel [-- Attachment #1: Type: text/plain, Size: 1265 bytes --] 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 [-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --] ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [Qemu-devel] [Qemu-block] [PATCH] block: Swap request limit definitions 2017-02-15 16:44 ` Kevin Wolf @ 2017-02-15 16:48 ` Max Reitz 2017-02-15 17:10 ` Kevin Wolf 0 siblings, 1 reply; 10+ messages in thread From: Max Reitz @ 2017-02-15 16:48 UTC (permalink / raw) To: Kevin Wolf; +Cc: Alberto Garcia, qemu-block, qemu-devel [-- Attachment #1: Type: text/plain, Size: 1826 bytes --] 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 [-- Attachment #2: OpenPGP digital signature --] [-- Type: application/pgp-signature, Size: 512 bytes --] ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [Qemu-devel] [Qemu-block] [PATCH] block: Swap request limit definitions 2017-02-15 16:48 ` Max Reitz @ 2017-02-15 17:10 ` Kevin Wolf 2017-02-15 17:15 ` Max Reitz 0 siblings, 1 reply; 10+ messages in thread From: Kevin Wolf @ 2017-02-15 17:10 UTC (permalink / raw) To: Max Reitz; +Cc: Alberto Garcia, qemu-block, qemu-devel [-- Attachment #1: Type: text/plain, Size: 2063 bytes --] 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 [-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --] ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [Qemu-devel] [Qemu-block] [PATCH] block: Swap request limit definitions 2017-02-15 17:10 ` Kevin Wolf @ 2017-02-15 17:15 ` Max Reitz 0 siblings, 0 replies; 10+ messages in thread From: Max Reitz @ 2017-02-15 17:15 UTC (permalink / raw) To: Kevin Wolf; +Cc: Alberto Garcia, qemu-block, qemu-devel [-- Attachment #1: Type: text/plain, Size: 2441 bytes --] 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 [-- Attachment #2: OpenPGP digital signature --] [-- Type: application/pgp-signature, Size: 512 bytes --] ^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2017-02-15 17:15 UTC | newest] Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2017-02-12 1:47 [Qemu-devel] [PATCH] block: Swap request limit definitions Max Reitz 2017-02-13 5:52 ` Fam Zheng 2017-02-13 8:39 ` [Qemu-devel] [Qemu-block] " Alberto Garcia 2017-02-13 17:13 ` Max Reitz 2017-02-14 9:52 ` Alberto Garcia 2017-02-15 13:42 ` Max Reitz 2017-02-15 16:44 ` Kevin Wolf 2017-02-15 16:48 ` Max Reitz 2017-02-15 17:10 ` Kevin Wolf 2017-02-15 17:15 ` Max Reitz
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.