| Message ID | 20181227160944.GA12190@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [git,pull] device mapper changes for 4.21 | expand |
The pull request you sent on Thu, 27 Dec 2018 11:09:44 -0500:
> git://git.kernel.org/pub/scm/linux/kernel/git/device-mapper/linux-dm.git tags/for-4.21/dm-changes
has been merged into torvalds/linux.git:
https://git.kernel.org/torvalds/c/4ed7bdc1eb4c82cf4bfdf6a94dd36fd695f6f387
Thank you!
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.
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
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
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
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.
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---
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
Hi Linus, The DM tree is based on the block tree for this cycle because a fair amount of my time was spent working on the percpu inflight IO counters changes and other block fixes (which impact DM) that are in Jens' block pull. The following changes since commit cd19181bf9ad4b7f40f2a4e0355d052109c76529: blk-mq: enable IO poll if .nr_queues of type poll > 0 (2018-12-17 21:35:07 -0700) are available in the Git repository at: git://git.kernel.org/pub/scm/linux/kernel/git/device-mapper/linux-dm.git tags/for-4.21/dm-changes for you to fetch changes up to c6d6e9b0f6b4201c77f2cea3964dd122697e3543: dm: do not allow readahead to limit IO size (2018-12-18 14:23:41 -0500) Please pull, thanks. Mike ---------------------------------------------------------------- - Eliminate a couple indirect calls from bio-based DM core. - Fix DM to allow reads that exceed readahead limits by setting io_pages in the backing_dev_info. - A couple code cleanups in request-based DM. - Fix various DM targets to check for device sector overflow if CONFIG_LBDAF is not set. - Use u64 instead of sector_t to store iv_offset in DM crypt; sector_t isn't large enough on 32bit when CONFIG_LBDAF is not set. - Performance fixes to DM's kcopyd and the snapshot target focused on limiting memory use and workqueue stalls. - Fix typos in the integrity and writecache targets. - Log which algorithm is used for dm-crypt's encryption and dm-integrity's hashing. - Fix false -EBUSY errors in DM raid target's handling of check/repair messages. - Fix DM flakey target's corrupt_bio_byte feature to reliably corrupt the Nth byte in a bio's payload. ---------------------------------------------------------------- AliOS system security (1): dm crypt: use u64 instead of sector_t to store iv_offset Colin Ian King (1): dm integrity: fix spelling mistake in workqueue name Eric Biggers (2): dm crypt: log the encryption algorithm implementation dm verity: log the hash algorithm implementation Heinz Mauelshagen (1): dm raid: fix false -EBUSY when handling check/repair message Jaegeuk Kim (1): dm: do not allow readahead to limit IO size Mike Snitzer (3): dm rq: remove unused arguments from rq_completed() dm: remove indirect calls from __send_changing_extent_only() dm rq: cleanup leftover code from recently removed q->mq_ops branching Mikulas Patocka (1): dm: avoid indirect call in __dm_make_request Milan Broz (1): dm: Check for device sector overflow if CONFIG_LBDAF is not set Nikos Tsironis (2): dm snapshot: Fix excessive memory usage and workqueue stalls dm kcopyd: Fix bug causing workqueue stalls Shenghui Wang (2): dm writecache: fix typo in error msg for creating writecache_flush_thread dm bufio: update comment in dm-bufio.c Sweet Tea (1): dm flakey: Properly corrupt multi-page bios. wuzhouhui (1): dm mpath: only flush workqueue when needed drivers/md/dm-bufio.c | 12 +++++------ drivers/md/dm-crypt.c | 17 ++++++++++++---- drivers/md/dm-delay.c | 2 +- drivers/md/dm-flakey.c | 35 +++++++++++++++++++++----------- drivers/md/dm-integrity.c | 2 +- drivers/md/dm-kcopyd.c | 19 +++++++++++++----- drivers/md/dm-linear.c | 2 +- drivers/md/dm-mpath.c | 6 ++++-- drivers/md/dm-raid.c | 3 +-- drivers/md/dm-raid1.c | 3 ++- drivers/md/dm-rq.c | 18 ++++++----------- drivers/md/dm-snap.c | 22 +++++++++++++++++++++ drivers/md/dm-table.c | 3 +++ drivers/md/dm-unstripe.c | 2 +- drivers/md/dm-verity-target.c | 9 +++++++++ drivers/md/dm-writecache.c | 2 +- drivers/md/dm.c | 46 ++++++++++++------------------------------- 17 files changed, 121 insertions(+), 82 deletions(-)