* Re: [git pull] device mapper changes for 4.21
@ 2018-12-30 9:06 ` Christoph Hellwig
0 siblings, 0 replies; 18+ messages in thread
From: Christoph Hellwig @ 2018-12-30 9:06 UTC (permalink / raw)
To: Mike Snitzer
Cc: Sweet Tea, wuzhouhui, AliOS system security, Nikos Tsironis,
Eric Biggers, Heinz Mauelshagen, Shenghui Wang, linux-block,
dm-devel, Mikulas Patocka, Jaegeuk Kim, Colin Ian King,
Linus Torvalds, Milan Broz, Alasdair G Kergon
On Thu, Dec 27, 2018 at 11:09:44AM -0500, Mike Snitzer wrote:
> - Fix various DM targets to check for device sector overflow if
> CONFIG_LBDAF is not set.
Question to Jens and Linus:
is there any good reason to keep the CONFIG_LBDAF=n option around?
Less than 2gig block devices seem to be an absolutele niche, and I
wonder if it is still worth maintaining the special case just for that.
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [git pull] device mapper changes for 4.21
2018-12-30 9:06 ` Christoph Hellwig
@ 2018-12-30 19:15 ` Mikulas Patocka
-1 siblings, 0 replies; 18+ messages in thread
From: Mikulas Patocka @ 2018-12-30 19:15 UTC (permalink / raw)
To: Christoph Hellwig
Cc: Mike Snitzer, Linus Torvalds, dm-devel, linux-block,
Alasdair G Kergon, AliOS system security, Colin Ian King,
Eric Biggers, Heinz Mauelshagen, Jaegeuk Kim, Milan Broz,
Nikos Tsironis, Shenghui Wang, Sweet Tea, wuzhouhui
On Sun, 30 Dec 2018, Christoph Hellwig wrote:
> On Thu, Dec 27, 2018 at 11:09:44AM -0500, Mike Snitzer wrote:
> > - Fix various DM targets to check for device sector overflow if
> > CONFIG_LBDAF is not set.
>
> Question to Jens and Linus:
>
> is there any good reason to keep the CONFIG_LBDAF=n option around?
> Less than 2gig block devices seem to be an absolutele niche, and I
> wonder if it is still worth maintaining the special case just for that.
The CONFIG_LBDAF limit is 2TiB, not 2GiB.
But you're right that 2TiB devices are common and that perhaps this option
should go away.
Mikulas
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [git pull] device mapper changes for 4.21
@ 2018-12-30 19:15 ` Mikulas Patocka
0 siblings, 0 replies; 18+ messages in thread
From: Mikulas Patocka @ 2018-12-30 19:15 UTC (permalink / raw)
To: Christoph Hellwig
Cc: Sweet Tea, wuzhouhui, AliOS system security, Nikos Tsironis,
Mike Snitzer, Eric Biggers, Heinz Mauelshagen, Shenghui Wang,
linux-block, dm-devel, Jaegeuk Kim, Colin Ian King,
Linus Torvalds, Milan Broz, Alasdair G Kergon
On Sun, 30 Dec 2018, Christoph Hellwig wrote:
> On Thu, Dec 27, 2018 at 11:09:44AM -0500, Mike Snitzer wrote:
> > - Fix various DM targets to check for device sector overflow if
> > CONFIG_LBDAF is not set.
>
> Question to Jens and Linus:
>
> is there any good reason to keep the CONFIG_LBDAF=n option around?
> Less than 2gig block devices seem to be an absolutele niche, and I
> wonder if it is still worth maintaining the special case just for that.
The CONFIG_LBDAF limit is 2TiB, not 2GiB.
But you're right that 2TiB devices are common and that perhaps this option
should go away.
Mikulas
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [git pull] device mapper changes for 4.21
2018-12-30 19:15 ` Mikulas Patocka
@ 2018-12-31 0:25 ` Linus Torvalds
-1 siblings, 0 replies; 18+ messages in thread
From: Linus Torvalds @ 2018-12-31 0:25 UTC (permalink / raw)
To: Mikulas Patocka
Cc: Christoph Hellwig, Mike Snitzer, dm-devel, linux-block,
Alasdair G Kergon, AliOS system security, Colin Ian King,
Eric Biggers, Heinz Mauelshagen, Jaegeuk Kim, Milan Broz,
Nikos Tsironis, Shenghui Wang, Sweet Tea, wuzhouhui
On Sun, Dec 30, 2018 at 11:15 AM Mikulas Patocka <mpatocka@redhat.com> wrote:
>
> But you're right that 2TiB devices are common and that perhaps this option
> should go away.
2TiB devices are definitely not common in the one situation where this
option might matter: small embedded devices.
I don't think the cost of 64 bit is in the arithmetic, but it might be
in some of the data structures.
But my gut feel is that it probably doesn't much matter, and we could
get rid of the config option without anybody ever noticing. I don't
think we have that many data structures with 'sector_t' in them.
We might try to first just force the option on, and see if anybody even cares.
Linus
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [git pull] device mapper changes for 4.21
@ 2018-12-31 0:25 ` Linus Torvalds
0 siblings, 0 replies; 18+ messages in thread
From: Linus Torvalds @ 2018-12-31 0:25 UTC (permalink / raw)
To: Mikulas Patocka
Cc: Sweet Tea, linux-block, AliOS system security, Nikos Tsironis,
Mike Snitzer, Eric Biggers, Heinz Mauelshagen, Shenghui Wang,
Christoph Hellwig, dm-devel, Jaegeuk Kim, wuzhouhui,
Colin Ian King, Milan Broz, Alasdair G Kergon
On Sun, Dec 30, 2018 at 11:15 AM Mikulas Patocka <mpatocka@redhat.com> wrote:
>
> But you're right that 2TiB devices are common and that perhaps this option
> should go away.
2TiB devices are definitely not common in the one situation where this
option might matter: small embedded devices.
I don't think the cost of 64 bit is in the arithmetic, but it might be
in some of the data structures.
But my gut feel is that it probably doesn't much matter, and we could
get rid of the config option without anybody ever noticing. I don't
think we have that many data structures with 'sector_t' in them.
We might try to first just force the option on, and see if anybody even cares.
Linus
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [git pull] device mapper changes for 4.21
2018-12-31 0:25 ` Linus Torvalds
@ 2019-01-03 7:27 ` Christoph Hellwig
-1 siblings, 0 replies; 18+ messages in thread
From: Christoph Hellwig @ 2019-01-03 7:27 UTC (permalink / raw)
To: Linus Torvalds
Cc: Mikulas Patocka, Christoph Hellwig, Mike Snitzer, dm-devel,
linux-block, Alasdair G Kergon, AliOS system security,
Colin Ian King, Eric Biggers, Heinz Mauelshagen, Jaegeuk Kim,
Milan Broz, Nikos Tsironis, Shenghui Wang, Sweet Tea, wuzhouhui
On Sun, Dec 30, 2018 at 04:25:46PM -0800, Linus Torvalds wrote:
> On Sun, Dec 30, 2018 at 11:15 AM Mikulas Patocka <mpatocka@redhat.com> wrote:
> >
> > But you're right that 2TiB devices are common and that perhaps this option
> > should go away.
>
> 2TiB devices are definitely not common in the one situation where this
> option might matter: small embedded devices.
>
> I don't think the cost of 64 bit is in the arithmetic, but it might be
> in some of the data structures.
>
> But my gut feel is that it probably doesn't much matter, and we could
> get rid of the config option without anybody ever noticing. I don't
> think we have that many data structures with 'sector_t' in them.
>
> We might try to first just force the option on, and see if anybody even cares.
Our smallest embedded devices use raw flash using the MTD subsystem,
and even that is using 64-bit size types everywhere. So I'd be really
surprised if it is an issue.
>
> Linus
---end quoted text---
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [git pull] device mapper changes for 4.21
@ 2019-01-03 7:27 ` Christoph Hellwig
0 siblings, 0 replies; 18+ messages in thread
From: Christoph Hellwig @ 2019-01-03 7:27 UTC (permalink / raw)
To: Linus Torvalds
Cc: Sweet Tea, linux-block, AliOS system security, Nikos Tsironis,
Mike Snitzer, Eric Biggers, Heinz Mauelshagen, Shenghui Wang,
Christoph Hellwig, dm-devel, Mikulas Patocka, Jaegeuk Kim,
wuzhouhui, Colin Ian King, Milan Broz, Alasdair G Kergon
On Sun, Dec 30, 2018 at 04:25:46PM -0800, Linus Torvalds wrote:
> On Sun, Dec 30, 2018 at 11:15 AM Mikulas Patocka <mpatocka@redhat.com> wrote:
> >
> > But you're right that 2TiB devices are common and that perhaps this option
> > should go away.
>
> 2TiB devices are definitely not common in the one situation where this
> option might matter: small embedded devices.
>
> I don't think the cost of 64 bit is in the arithmetic, but it might be
> in some of the data structures.
>
> But my gut feel is that it probably doesn't much matter, and we could
> get rid of the config option without anybody ever noticing. I don't
> think we have that many data structures with 'sector_t' in them.
>
> We might try to first just force the option on, and see if anybody even cares.
Our smallest embedded devices use raw flash using the MTD subsystem,
and even that is using 64-bit size types everywhere. So I'd be really
surprised if it is an issue.
>
> Linus
---end quoted text---
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [git pull] device mapper changes for 4.21
2019-01-03 7:27 ` Christoph Hellwig
@ 2019-01-03 7:45 ` Milan Broz
-1 siblings, 0 replies; 18+ messages in thread
From: Milan Broz @ 2019-01-03 7:45 UTC (permalink / raw)
To: Christoph Hellwig, Linus Torvalds
Cc: Mikulas Patocka, Mike Snitzer, dm-devel, linux-block,
Alasdair G Kergon, AliOS system security, Colin Ian King,
Eric Biggers, Heinz Mauelshagen, Jaegeuk Kim, Milan Broz,
Nikos Tsironis, Shenghui Wang, Sweet Tea, wuzhouhui
On 03/01/2019 08:27, Christoph Hellwig wrote:
> On Sun, Dec 30, 2018 at 04:25:46PM -0800, Linus Torvalds wrote:
>> On Sun, Dec 30, 2018 at 11:15 AM Mikulas Patocka <mpatocka@redhat.com> wrote:
>>>
>>> But you're right that 2TiB devices are common and that perhaps this option
>>> should go away.
>>
>> 2TiB devices are definitely not common in the one situation where this
>> option might matter: small embedded devices.
>>
>> I don't think the cost of 64 bit is in the arithmetic, but it might be
>> in some of the data structures.
>>
>> But my gut feel is that it probably doesn't much matter, and we could
>> get rid of the config option without anybody ever noticing. I don't
>> think we have that many data structures with 'sector_t' in them.
>>
>> We might try to first just force the option on, and see if anybody even cares.
>
> Our smallest embedded devices use raw flash using the MTD subsystem,
> and even that is using 64-bit size types everywhere. So I'd be really
> surprised if it is an issue.
I agree with Christoph here.
(I fixed some CONFIG_LBDAF problems in DM in this pull request because
the code was apparently wrong, but it was a pain to see all these possible
sector overflow checks...)
If it helps anything, we require 64-bit calculations for cryptsetup userspace for >5 years
(you cannot compile it with 32bit support; everything uses 64bit, including these
DM table sector calculations for kernel) and NOBODY complained since we enforced it.
(Ok, it is not a hot path, but....)
Please, if possible, go with 64-bit sector size types by default in future.
Thanks,
Milan
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [git pull] device mapper changes for 4.21
@ 2019-01-03 7:45 ` Milan Broz
0 siblings, 0 replies; 18+ messages in thread
From: Milan Broz @ 2019-01-03 7:45 UTC (permalink / raw)
To: Christoph Hellwig, Linus Torvalds
Cc: Sweet Tea, wuzhouhui, AliOS system security, Nikos Tsironis,
Mike Snitzer, Eric Biggers, Heinz Mauelshagen, Shenghui Wang,
linux-block, dm-devel, Mikulas Patocka, Jaegeuk Kim,
Colin Ian King, Milan Broz, Alasdair G Kergon
On 03/01/2019 08:27, Christoph Hellwig wrote:
> On Sun, Dec 30, 2018 at 04:25:46PM -0800, Linus Torvalds wrote:
>> On Sun, Dec 30, 2018 at 11:15 AM Mikulas Patocka <mpatocka@redhat.com> wrote:
>>>
>>> But you're right that 2TiB devices are common and that perhaps this option
>>> should go away.
>>
>> 2TiB devices are definitely not common in the one situation where this
>> option might matter: small embedded devices.
>>
>> I don't think the cost of 64 bit is in the arithmetic, but it might be
>> in some of the data structures.
>>
>> But my gut feel is that it probably doesn't much matter, and we could
>> get rid of the config option without anybody ever noticing. I don't
>> think we have that many data structures with 'sector_t' in them.
>>
>> We might try to first just force the option on, and see if anybody even cares.
>
> Our smallest embedded devices use raw flash using the MTD subsystem,
> and even that is using 64-bit size types everywhere. So I'd be really
> surprised if it is an issue.
I agree with Christoph here.
(I fixed some CONFIG_LBDAF problems in DM in this pull request because
the code was apparently wrong, but it was a pain to see all these possible
sector overflow checks...)
If it helps anything, we require 64-bit calculations for cryptsetup userspace for >5 years
(you cannot compile it with 32bit support; everything uses 64bit, including these
DM table sector calculations for kernel) and NOBODY complained since we enforced it.
(Ok, it is not a hot path, but....)
Please, if possible, go with 64-bit sector size types by default in future.
Thanks,
Milan
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [git pull] device mapper changes for 4.21
2018-12-30 9:06 ` Christoph Hellwig
@ 2018-12-30 19:40 ` James Bottomley
-1 siblings, 0 replies; 18+ messages in thread
From: James Bottomley @ 2018-12-30 19:40 UTC (permalink / raw)
To: Christoph Hellwig, Mike Snitzer
Cc: Linus Torvalds, dm-devel, linux-block, Alasdair G Kergon,
AliOS system security, Colin Ian King, Eric Biggers,
Heinz Mauelshagen, Jaegeuk Kim, Mikulas Patocka, Milan Broz,
Nikos Tsironis, Shenghui Wang, Sweet Tea, wuzhouhui, linux-arch
On Sun, 2018-12-30 at 01:06 -0800, Christoph Hellwig wrote:
> On Thu, Dec 27, 2018 at 11:09:44AM -0500, Mike Snitzer wrote:
> > - Fix various DM targets to check for device sector overflow if
> > CONFIG_LBDAF is not set.
>
> Question to Jens and Linus:
>
> is there any good reason to keep the CONFIG_LBDAF=n option around?
> Less than 2gig block devices seem to be an absolutele niche, and I
> wonder if it is still worth maintaining the special case just for
> that.
It's really a question for embedded isn't it? since they're the ones
with this set to 'n' in their default configs (linux-arch cc added).
What LBDAF=n does is make sector_t and blkcnt_t unsigned long instead
of u64. The maintenance burden to us is we have to use sector_div
instead of do_div in the code and remember that sector_t may be 32 bit
(the current problem in LVM). The benefit to embedded architectures is
that they don't have to do 64 bit arithmetic for every block
transaction and the price is they can't support block devices larger
than 2TB (not 2GB, although the AF part means we can't support file
sizes bigger than 2GB).
So the first question is: is there an embedded platform where anyone
thinks the cost of the 64 bit arithmetic is important? and if there is
can they benchmark it so we get an idea of the value of keeping LBDAF
(i.e. if it's just a few percent then likely it's not worth it, if it's
in the tens of percent, it might be).
James
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [git pull] device mapper changes for 4.21
@ 2018-12-30 19:40 ` James Bottomley
0 siblings, 0 replies; 18+ messages in thread
From: James Bottomley @ 2018-12-30 19:40 UTC (permalink / raw)
To: Christoph Hellwig, Mike Snitzer
Cc: Tea, wuzhouhui, Heinz, system security, Tsironis, Eric Biggers,
Mauelshagen, Sweet, Nikos, Shenghui Wang, linux-block, dm-devel,
Patocka, Mikulas, Jaegeuk Kim, Colin Ian King, linux-arch,
Linus Torvalds, Milan Broz, Alasdair G Kergon, AliOS
On Sun, 2018-12-30 at 01:06 -0800, Christoph Hellwig wrote:
> On Thu, Dec 27, 2018 at 11:09:44AM -0500, Mike Snitzer wrote:
> > - Fix various DM targets to check for device sector overflow if
> > CONFIG_LBDAF is not set.
>
> Question to Jens and Linus:
>
> is there any good reason to keep the CONFIG_LBDAF=n option around?
> Less than 2gig block devices seem to be an absolutele niche, and I
> wonder if it is still worth maintaining the special case just for
> that.
It's really a question for embedded isn't it? since they're the ones
with this set to 'n' in their default configs (linux-arch cc added).
What LBDAF=n does is make sector_t and blkcnt_t unsigned long instead
of u64. The maintenance burden to us is we have to use sector_div
instead of do_div in the code and remember that sector_t may be 32 bit
(the current problem in LVM). The benefit to embedded architectures is
that they don't have to do 64 bit arithmetic for every block
transaction and the price is they can't support block devices larger
than 2TB (not 2GB, although the AF part means we can't support file
sizes bigger than 2GB).
So the first question is: is there an embedded platform where anyone
thinks the cost of the 64 bit arithmetic is important? and if there is
can they benchmark it so we get an idea of the value of keeping LBDAF
(i.e. if it's just a few percent then likely it's not worth it, if it's
in the tens of percent, it might be).
James
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [git pull] device mapper changes for 4.21
2018-12-30 9:06 ` Christoph Hellwig
@ 2018-12-31 20:10 ` Jens Axboe
-1 siblings, 0 replies; 18+ messages in thread
From: Jens Axboe @ 2018-12-31 20:10 UTC (permalink / raw)
To: Christoph Hellwig, Mike Snitzer
Cc: Linus Torvalds, dm-devel, linux-block, Alasdair G Kergon,
AliOS system security, Colin Ian King, Eric Biggers,
Heinz Mauelshagen, Jaegeuk Kim, Mikulas Patocka, Milan Broz,
Nikos Tsironis, Shenghui Wang, Sweet Tea, wuzhouhui
On 12/30/18 2:06 AM, Christoph Hellwig wrote:
> On Thu, Dec 27, 2018 at 11:09:44AM -0500, Mike Snitzer wrote:
>> - Fix various DM targets to check for device sector overflow if
>> CONFIG_LBDAF is not set.
>
> Question to Jens and Linus:
>
> is there any good reason to keep the CONFIG_LBDAF=n option around?
> Less than 2gig block devices seem to be an absolutele niche, and I
> wonder if it is still worth maintaining the special case just for that.
I'd be fine with removing it, I seriously doubt that the extended
math is going to be noticeable at all.
I'll try and run some null_blk testing as micro benchmarks when
I'm back in a few days.
--
Jens Axboe
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [git pull] device mapper changes for 4.21
@ 2018-12-31 20:10 ` Jens Axboe
0 siblings, 0 replies; 18+ messages in thread
From: Jens Axboe @ 2018-12-31 20:10 UTC (permalink / raw)
To: Christoph Hellwig, Mike Snitzer
Cc: Sweet Tea, wuzhouhui, AliOS system security, Nikos Tsironis,
Eric Biggers, Heinz Mauelshagen, Shenghui Wang, linux-block,
dm-devel, Mikulas Patocka, Jaegeuk Kim, Colin Ian King,
Linus Torvalds, Milan Broz, Alasdair G Kergon
On 12/30/18 2:06 AM, Christoph Hellwig wrote:
> On Thu, Dec 27, 2018 at 11:09:44AM -0500, Mike Snitzer wrote:
>> - Fix various DM targets to check for device sector overflow if
>> CONFIG_LBDAF is not set.
>
> Question to Jens and Linus:
>
> is there any good reason to keep the CONFIG_LBDAF=n option around?
> Less than 2gig block devices seem to be an absolutele niche, and I
> wonder if it is still worth maintaining the special case just for that.
I'd be fine with removing it, I seriously doubt that the extended
math is going to be noticeable at all.
I'll try and run some null_blk testing as micro benchmarks when
I'm back in a few days.
--
Jens Axboe
^ permalink raw reply [flat|nested] 18+ messages in thread