Re: [dm-devel] dm thin: superblock may write succeed before other metadata blocks because of wirting metadata in async mode.
On Tue, Jun 19, 2018 at 11:00:32AM -0400, Mike Snitzer wrote: > > On Tue, Jun 19 2018 at 10:43am -0400, > Joe Thornber wrote: > > > On Tue, Jun 19, 2018 at 09:11:06AM -0400, Mike Snitzer wrote: > > > On Mon, May 21 2018 at 8:53pm -0400, > > > Monty Pavel wrote: > > > > > > > > > > > If dm_bufio_write_dirty_buffers func is called by __commit_transaction > > > > func and power loss happens during executing it, coincidencely > > > > superblock wrote correctly but some metadata blocks didn't. The reason > > > > is we write all metadata in async mode. We can guarantee that we send > > > > superblock after other blocks but we cannot guarantee that superblock > > > > write completely early than other blocks. > > > > So, We need to commit other metadata blocks before change superblock. > > > > > > > > Signed-off-by: Monty Pavel > > > > --- > > > > drivers/md/dm-thin-metadata.c |8 > > > > 1 files changed, 8 insertions(+), 0 deletions(-) > > > > > > > > diff --git a/drivers/md/dm-thin-metadata.c > > > > b/drivers/md/dm-thin-metadata.c > > > > index 36ef284..897d7d6 100644 > > > > --- a/drivers/md/dm-thin-metadata.c > > > > +++ b/drivers/md/dm-thin-metadata.c > > > > @@ -813,6 +813,14 @@ static int __commit_transaction(struct > > > > dm_pool_metadata *pmd) > > > > if (r) > > > > return r; > > > > > > > > + r = dm_tm_commit(pmd->tm, sblock); > > > > + if (r) > > > > + return r; > > > > + > > > > + r = superblock_lock(pmd, &sblock); > > > > + if (r) > > > > + return r; > > > > + > > > > disk_super = dm_block_data(sblock); > > > > disk_super->time = cpu_to_le32(pmd->time); > > > > disk_super->data_mapping_root = cpu_to_le64(pmd->root); > > > > I don't believe you've tested this; sblock is passed to dm_tm_commit() > > uninitialised, and you didn't even bother to remove the later (and correct) > > call to dm_tm_commit(). > > I pointed out to Joe that the patch, in isolation, is decieving. It > _looks_ like sblock may be uninitialized, etc. But once the patch is > applied and you look at the entirety of __commit_transaction() it is > clear that you're reusing the existing superblock_lock() to safely > accomplish your additional call to dm_tm_commit(). > > > What is the issue that started you looking in this area? > > Right, as my previous reply asked: please clarify if you _know_ your > patch fixes an actual problem you've experienced. The more details the > better. > > Thanks, > Mike > Hi, Mike and Joe. Thanks for your reply. I read __commit_transaction many times and didn't find any problem of 2-phase commit. I use md-raid1(PCIe nvme and md-raid5) in write-behind mode to store dm-thin metadata. Test case: 1. I do copy-diff test on thin device and then reboot my machine. 2. Rebuild our device stack and exec "vgchang -ay". The thin-pool can not be established(details_root become a bitmap node and metadata's bitmap_root become a btree_node). Thanks, Monty -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel
Re: [dm-devel] dm thin: superblock may write succeed before other metadata blocks because of wirting metadata in async mode.
On Tue, Jun 19 2018 at 10:43am -0400, Joe Thornber wrote: > On Tue, Jun 19, 2018 at 09:11:06AM -0400, Mike Snitzer wrote: > > On Mon, May 21 2018 at 8:53pm -0400, > > Monty Pavel wrote: > > > > > > > > If dm_bufio_write_dirty_buffers func is called by __commit_transaction > > > func and power loss happens during executing it, coincidencely > > > superblock wrote correctly but some metadata blocks didn't. The reason > > > is we write all metadata in async mode. We can guarantee that we send > > > superblock after other blocks but we cannot guarantee that superblock > > > write completely early than other blocks. > > > So, We need to commit other metadata blocks before change superblock. > > > > > > Signed-off-by: Monty Pavel > > > --- > > > drivers/md/dm-thin-metadata.c |8 > > > 1 files changed, 8 insertions(+), 0 deletions(-) > > > > > > diff --git a/drivers/md/dm-thin-metadata.c b/drivers/md/dm-thin-metadata.c > > > index 36ef284..897d7d6 100644 > > > --- a/drivers/md/dm-thin-metadata.c > > > +++ b/drivers/md/dm-thin-metadata.c > > > @@ -813,6 +813,14 @@ static int __commit_transaction(struct > > > dm_pool_metadata *pmd) > > > if (r) > > > return r; > > > > > > + r = dm_tm_commit(pmd->tm, sblock); > > > + if (r) > > > + return r; > > > + > > > + r = superblock_lock(pmd, &sblock); > > > + if (r) > > > + return r; > > > + > > > disk_super = dm_block_data(sblock); > > > disk_super->time = cpu_to_le32(pmd->time); > > > disk_super->data_mapping_root = cpu_to_le64(pmd->root); > > I don't believe you've tested this; sblock is passed to dm_tm_commit() > uninitialised, and you didn't even bother to remove the later (and correct) > call to dm_tm_commit(). I pointed out to Joe that the patch, in isolation, is decieving. It _looks_ like sblock may be uninitialized, etc. But once the patch is applied and you look at the entirety of __commit_transaction() it is clear that you're reusing the existing superblock_lock() to safely accomplish your additional call to dm_tm_commit(). > What is the issue that started you looking in this area? Right, as my previous reply asked: please clarify if you _know_ your patch fixes an actual problem you've experienced. The more details the better. Thanks, Mike -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel
Re: [dm-devel] dm thin: superblock may write succeed before other metadata blocks because of wirting metadata in async mode.
On Tue, Jun 19, 2018 at 09:11:06AM -0400, Mike Snitzer wrote: > On Mon, May 21 2018 at 8:53pm -0400, > Monty Pavel wrote: > > > > > If dm_bufio_write_dirty_buffers func is called by __commit_transaction > > func and power loss happens during executing it, coincidencely > > superblock wrote correctly but some metadata blocks didn't. The reason > > is we write all metadata in async mode. We can guarantee that we send > > superblock after other blocks but we cannot guarantee that superblock > > write completely early than other blocks. > > So, We need to commit other metadata blocks before change superblock. > > > > Signed-off-by: Monty Pavel > > --- > > drivers/md/dm-thin-metadata.c |8 > > 1 files changed, 8 insertions(+), 0 deletions(-) > > > > diff --git a/drivers/md/dm-thin-metadata.c b/drivers/md/dm-thin-metadata.c > > index 36ef284..897d7d6 100644 > > --- a/drivers/md/dm-thin-metadata.c > > +++ b/drivers/md/dm-thin-metadata.c > > @@ -813,6 +813,14 @@ static int __commit_transaction(struct > > dm_pool_metadata *pmd) > > if (r) > > return r; > > > > + r = dm_tm_commit(pmd->tm, sblock); > > + if (r) > > + return r; > > + > > + r = superblock_lock(pmd, &sblock); > > + if (r) > > + return r; > > + > > disk_super = dm_block_data(sblock); > > disk_super->time = cpu_to_le32(pmd->time); > > disk_super->data_mapping_root = cpu_to_le64(pmd->root); I don't believe you've tested this; sblock is passed to dm_tm_commit() uninitialised, and you didn't even bother to remove the later (and correct) call to dm_tm_commit(). See dm-transaction-manager.h for an explanation of how the 2-phase commit works. What is the issue that started you looking in this area? - Joe -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel
Re: [dm-devel] dm thin: superblock may write succeed before other metadata blocks because of wirting metadata in async mode.
On Mon, May 21 2018 at 8:53pm -0400, Monty Pavel wrote: > > If dm_bufio_write_dirty_buffers func is called by __commit_transaction > func and power loss happens during executing it, coincidencely > superblock wrote correctly but some metadata blocks didn't. The reason > is we write all metadata in async mode. We can guarantee that we send > superblock after other blocks but we cannot guarantee that superblock > write completely early than other blocks. > So, We need to commit other metadata blocks before change superblock. > > Signed-off-by: Monty Pavel > --- > drivers/md/dm-thin-metadata.c |8 > 1 files changed, 8 insertions(+), 0 deletions(-) > > diff --git a/drivers/md/dm-thin-metadata.c b/drivers/md/dm-thin-metadata.c > index 36ef284..897d7d6 100644 > --- a/drivers/md/dm-thin-metadata.c > +++ b/drivers/md/dm-thin-metadata.c > @@ -813,6 +813,14 @@ static int __commit_transaction(struct dm_pool_metadata > *pmd) > if (r) > return r; > > + r = dm_tm_commit(pmd->tm, sblock); > + if (r) > + return r; > + > + r = superblock_lock(pmd, &sblock); > + if (r) > + return r; > + > disk_super = dm_block_data(sblock); > disk_super->time = cpu_to_le32(pmd->time); > disk_super->data_mapping_root = cpu_to_le64(pmd->root); > -- > 1.7.1 Have you actually found this patch to be effective? It should be unnecessary. But I must admit that in looking at the related code I couldn't convince myself it was. But then Joe pointed me to this comment block from dm-transaction-manager.h: /* * We use a 2-phase commit here. * * i) Make all changes for the transaction *except* for the superblock. * Then call dm_tm_pre_commit() to flush them to disk. * * ii) Lock your superblock. Update. Then call dm_tm_commit() which will * unlock the superblock and flush it. No other blocks should be updated * during this period. Care should be taken to never unlock a partially * updated superblock; perform any operations that could fail *before* you * take the superblock lock. */ int dm_tm_pre_commit(struct dm_transaction_manager *tm); int dm_tm_commit(struct dm_transaction_manager *tm, struct dm_block *superblock); So given __commit_transaction() is using dm_tm_pre_commit() prior to the dm_tm_commit() to flush the superblock -- it would seem that there isn't any conceptual potential for corruption. If you've found the dm_tm_pre_commit() to be lacking (whereby not all metadata getting flushed to disk before the superblock) then please explain your findings. Thanks, Mike -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel
Re: [dm-devel] dm bufio: Reduce dm_bufio_lock contention
On Mon 18-06-18 18:11:26, Mikulas Patocka wrote: [...] > I grepped the kernel for __GFP_NORETRY and triaged them. I found 16 cases > without a fallback - those are bugs that make various functions randomly > return -ENOMEM. Well, maybe those are just optimistic attempts to allocate memory and have a fallback somewhere else. So I would be careful calling them outright bugs. But maybe you are right. > Most of the callers provide callback. > > There is another strange flag - __GFP_RETRY_MAYFAIL - it provides two > different functions - if the allocation is larger than > PAGE_ALLOC_COSTLY_ORDER, it retries the allocation as if it were smaller. > If the allocations is smaller than PAGE_ALLOC_COSTLY_ORDER, > __GFP_RETRY_MAYFAIL will avoid the oom killer (larger order allocations > don't trigger the oom killer at all). Well, the primary purpose of this flag is to provide a consistent failure behavior for all requests regardless of the size. > So, perhaps __GFP_RETRY_MAYFAIL could be used instead of __GFP_NORETRY in > the cases where the caller wants to avoid trigerring the oom killer (the > problem is that __GFP_NORETRY causes random failure even in no-oom > situations but __GFP_RETRY_MAYFAIL doesn't). myabe yes. > So my suggestion is - fix these obvious bugs when someone allocates memory > with __GFP_NORETRY without any fallback - and then, __GFP_NORETRY could be > just changed to return NULL instead of sleeping. No real objection to fixing wrong __GFP_NORETRY usage. But __GFP_NORETRY can sleep. Nothing will really change in that regards. It does a reclaim and that _might_ sleep. But seriously, isn't the best way around the throttling issue to use PF_LESS_THROTTLE? -- Michal Hocko SUSE Labs -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel
Re: [dm-devel] [PATCH 3/6] exofs: use bio_clone_fast in _write_mirror
On 19/06/18 07:52, Christoph Hellwig wrote:
> The mirroring code never changes the bio data or biovecs. This means
> we can reuse the biovec allocation easily instead of duplicating it.
>
> Signed-off-by: Christoph Hellwig
Thank you yes that's much better
ACK-by Boaz Harrosh
> ---
> fs/exofs/ore.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/fs/exofs/ore.c b/fs/exofs/ore.c
> index 1b8b44637e70..5331a15a61f1 100644
> --- a/fs/exofs/ore.c
> +++ b/fs/exofs/ore.c
> @@ -873,8 +873,8 @@ static int _write_mirror(struct ore_io_state *ios, int
> cur_comp)
> struct bio *bio;
>
> if (per_dev != master_dev) {
> - bio = bio_clone_kmalloc(master_dev->bio,
> - GFP_KERNEL);
> + bio = bio_clone_fast(master_dev->bio,
> + GFP_KERNEL, NULL);
> if (unlikely(!bio)) {
> ORE_DBGMSG(
> "Failed to allocate BIO
> size=%u\n",
>
--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel
Re: [dm-devel] [PATCH 2/6] bcache: don't clone bio in bch_data_verify
On 2018/6/19 12:52 PM, Christoph Hellwig wrote:
> We immediately overwrite the biovec array, so instead just allocate
> a new bio and copy over the disk, setor and size.
>
> Signed-off-by: Christoph Hellwig
It looks good to me. Acked-by: Coly Li
Thanks.
Coly Li
> ---
> drivers/md/bcache/debug.c | 6 +-
> 1 file changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/md/bcache/debug.c b/drivers/md/bcache/debug.c
> index d030ce3025a6..04d146711950 100644
> --- a/drivers/md/bcache/debug.c
> +++ b/drivers/md/bcache/debug.c
> @@ -110,11 +110,15 @@ void bch_data_verify(struct cached_dev *dc, struct bio
> *bio)
> struct bio_vec bv, cbv;
> struct bvec_iter iter, citer = { 0 };
>
> - check = bio_clone_kmalloc(bio, GFP_NOIO);
> + check = bio_kmalloc(GFP_NOIO, bio_segments(bio));
> if (!check)
> return;
> + check->bi_disk = bio->bi_disk;
> check->bi_opf = REQ_OP_READ;
> + check->bi_iter.bi_sector = bio->bi_iter.bi_sector;
> + check->bi_iter.bi_size = bio->bi_iter.bi_size;
>
> + bch_bio_map(check, NULL);
> if (bch_bio_alloc_pages(check, GFP_NOIO))
> goto out_put;
>
>
--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel
[dm-devel] [PATCH v3 2/5] bitmap: Drop unnecessary 0 check for u32 array operations
The nbits == 0 is safe to be supplied to the function body, so,
remove unnecessary checks in bitmap_to_arr32() and bitmap_from_arr32().
Acked-by: Yury Norov
Signed-off-by: Andy Shevchenko
---
lib/bitmap.c | 9 +
1 file changed, 1 insertion(+), 8 deletions(-)
diff --git a/lib/bitmap.c b/lib/bitmap.c
index 58f9750e49c6..33e95cd359a2 100644
--- a/lib/bitmap.c
+++ b/lib/bitmap.c
@@ -1132,14 +1132,10 @@ EXPORT_SYMBOL(bitmap_copy_le);
* @buf: array of u32 (in host byte order), the source bitmap
* @nbits: number of bits in @bitmap
*/
-void bitmap_from_arr32(unsigned long *bitmap, const u32 *buf,
- unsigned int nbits)
+void bitmap_from_arr32(unsigned long *bitmap, const u32 *buf, unsigned int
nbits)
{
unsigned int i, halfwords;
- if (!nbits)
- return;
-
halfwords = DIV_ROUND_UP(nbits, 32);
for (i = 0; i < halfwords; i++) {
bitmap[i/2] = (unsigned long) buf[i];
@@ -1163,9 +1159,6 @@ void bitmap_to_arr32(u32 *buf, const unsigned long
*bitmap, unsigned int nbits)
{
unsigned int i, halfwords;
- if (!nbits)
- return;
-
halfwords = DIV_ROUND_UP(nbits, 32);
for (i = 0; i < halfwords; i++) {
buf[i] = (u32) (bitmap[i/2] & UINT_MAX);
--
2.17.1
--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel
[dm-devel] [PATCH v3 1/5] md: Avoid namespace collision with bitmap API
bitmap API (include/linux/bitmap.h) has 'bitmap' prefix for its methods.
On the other hand MD bitmap API is special case.
Adding 'md' prefix to it to avoid name space collision.
No functional changes intended.
Signed-off-by: Andy Shevchenko
---
drivers/md/dm-raid.c | 6 +-
drivers/md/md-bitmap.c| 301 +-
drivers/md/md-bitmap.h| 46 +--
drivers/md/md-cluster.c | 16 +-
drivers/md/md.c | 44 +--
.../md/persistent-data/dm-space-map-common.c | 12 +-
drivers/md/raid1.c| 20 +-
drivers/md/raid10.c | 26 +-
drivers/md/raid5-cache.c | 2 +-
drivers/md/raid5.c| 24 +-
10 files changed, 244 insertions(+), 253 deletions(-)
diff --git a/drivers/md/dm-raid.c b/drivers/md/dm-raid.c
index ab13fcec3fca..71d158560e69 100644
--- a/drivers/md/dm-raid.c
+++ b/drivers/md/dm-raid.c
@@ -3859,7 +3859,7 @@ static int __load_dirty_region_bitmap(struct raid_set *rs)
/* Try loading the bitmap unless "raid0", which does not have one */
if (!rs_is_raid0(rs) &&
!test_and_set_bit(RT_FLAG_RS_BITMAP_LOADED, &rs->runtime_flags)) {
- r = bitmap_load(&rs->md);
+ r = md_bitmap_load(&rs->md);
if (r)
DMERR("Failed to load bitmap");
}
@@ -3987,8 +3987,8 @@ static int raid_preresume(struct dm_target *ti)
/* Resize bitmap to adjust to changed region size (aka MD bitmap
chunksize) */
if (test_bit(RT_FLAG_RS_BITMAP_LOADED, &rs->runtime_flags) &&
mddev->bitmap &&
mddev->bitmap_info.chunksize !=
to_bytes(rs->requested_bitmap_chunk_sectors)) {
- r = bitmap_resize(mddev->bitmap, mddev->dev_sectors,
- to_bytes(rs->requested_bitmap_chunk_sectors),
0);
+ r = md_bitmap_resize(mddev->bitmap, mddev->dev_sectors,
+
to_bytes(rs->requested_bitmap_chunk_sectors), 0);
if (r)
DMERR("Failed to resize bitmap");
}
diff --git a/drivers/md/md-bitmap.c b/drivers/md/md-bitmap.c
index f983c3fdf204..307e7a90d78f 100644
--- a/drivers/md/md-bitmap.c
+++ b/drivers/md/md-bitmap.c
@@ -46,8 +46,8 @@ static inline char *bmname(struct bitmap *bitmap)
* if we find our page, we increment the page's refcount so that it stays
* allocated while we're using it
*/
-static int bitmap_checkpage(struct bitmap_counts *bitmap,
- unsigned long page, int create, int no_hijack)
+static int md_bitmap_checkpage(struct bitmap_counts *bitmap,
+ unsigned long page, int create, int no_hijack)
__releases(bitmap->lock)
__acquires(bitmap->lock)
{
@@ -115,7 +115,7 @@ __acquires(bitmap->lock)
/* if page is completely empty, put it back on the free list, or dealloc it */
/* if page was hijacked, unmark the flag so it might get alloced next time */
/* Note: lock should be held when calling this */
-static void bitmap_checkfree(struct bitmap_counts *bitmap, unsigned long page)
+static void md_bitmap_checkfree(struct bitmap_counts *bitmap, unsigned long
page)
{
char *ptr;
@@ -280,7 +280,7 @@ static int write_sb_page(struct bitmap *bitmap, struct page
*page, int wait)
return -EINVAL;
}
-static void bitmap_file_kick(struct bitmap *bitmap);
+static void md_bitmap_file_kick(struct bitmap *bitmap);
/*
* write out a page to a file
*/
@@ -310,7 +310,7 @@ static void write_page(struct bitmap *bitmap, struct page
*page, int wait)
atomic_read(&bitmap->pending_writes)==0);
}
if (test_bit(BITMAP_WRITE_ERROR, &bitmap->flags))
- bitmap_file_kick(bitmap);
+ md_bitmap_file_kick(bitmap);
}
static void end_bitmap_write(struct buffer_head *bh, int uptodate)
@@ -421,11 +421,11 @@ static int read_page(struct file *file, unsigned long
index,
*/
/*
- * bitmap_wait_writes() should be called before writing any bitmap
+ * md_bitmap_wait_writes() should be called before writing any bitmap
* blocks, to ensure previous writes, particularly from
- * bitmap_daemon_work(), have completed.
+ * md_bitmap_daemon_work(), have completed.
*/
-static void bitmap_wait_writes(struct bitmap *bitmap)
+static void md_bitmap_wait_writes(struct bitmap *bitmap)
{
if (bitmap->storage.file)
wait_event(bitmap->write_wait,
@@ -443,7 +443,7 @@ static void bitmap_wait_writes(struct bitmap *bitmap)
/* update the event counter and sync the superblock to disk */
-void bitmap_update_sb(struct bitmap *bitmap)
+void md_bitmap_update_sb(struct bitmap *bitmap)
{
bitmap_super_t *sb;
@@ -476,10 +476,10 @@ void bitmap_update_sb(struct bitmap *bitmap)
kunmap_atomic(sb);
write_page(bitmap, bitmap->stor
[dm-devel] [PATCH v3 3/5] bitmap: Add bitmap_alloc(), bitmap_zalloc() and bitmap_free()
A lot of code become ugly because of open coding allocations for bitmaps.
Introduce three helpers to allow users be more clear of intention
and keep their code neat.
Signed-off-by: Andy Shevchenko
---
include/linux/bitmap.h | 8
lib/bitmap.c | 19 +++
2 files changed, 27 insertions(+)
diff --git a/include/linux/bitmap.h b/include/linux/bitmap.h
index 1ee46f492267..acf5e8df3504 100644
--- a/include/linux/bitmap.h
+++ b/include/linux/bitmap.h
@@ -104,6 +104,14 @@
* contain all bit positions from 0 to 'bits' - 1.
*/
+/*
+ * Allocation and deallocation of bitmap.
+ * Provided in lib/bitmap.c to avoid circular dependency.
+ */
+extern unsigned long *bitmap_alloc(unsigned int nbits, gfp_t flags);
+extern unsigned long *bitmap_zalloc(unsigned int nbits, gfp_t flags);
+extern void bitmap_free(const unsigned long *bitmap);
+
/*
* lib/bitmap.c provides these functions:
*/
diff --git a/lib/bitmap.c b/lib/bitmap.c
index 33e95cd359a2..09acf2fd6a35 100644
--- a/lib/bitmap.c
+++ b/lib/bitmap.c
@@ -13,6 +13,7 @@
#include
#include
#include
+#include
#include
#include
@@ -1125,6 +1126,24 @@ void bitmap_copy_le(unsigned long *dst, const unsigned
long *src, unsigned int n
EXPORT_SYMBOL(bitmap_copy_le);
#endif
+unsigned long *bitmap_alloc(unsigned int nbits, gfp_t flags)
+{
+ return kmalloc_array(BITS_TO_LONGS(nbits), sizeof(unsigned long),
flags);
+}
+EXPORT_SYMBOL(bitmap_alloc);
+
+unsigned long *bitmap_zalloc(unsigned int nbits, gfp_t flags)
+{
+ return bitmap_alloc(nbits, flags | __GFP_ZERO);
+}
+EXPORT_SYMBOL(bitmap_zalloc);
+
+void bitmap_free(const unsigned long *bitmap)
+{
+ kfree(bitmap);
+}
+EXPORT_SYMBOL(bitmap_free);
+
#if BITS_PER_LONG == 64
/**
* bitmap_from_arr32 - copy the contents of u32 array of bits to bitmap
--
2.17.1
--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel
[dm-devel] [PATCH v3 0/5] bitmap: Introduce alloc/free helpers
A lot of code is using allocation of bitmaps using BITS_PER_LONG() macro and sizeof(unsigned long) operator. The readability suffers because of this. The series introduces three helpers, i.e. bitmap_alloc(), bitmap_zalloc() and bitmap_free(), to make it more cleaner. Patch 1 is a preparatory to avoid namespace collisions between bitmap API and MD bitmap. No functional changes intended. Patch 2 is just orphaned from previous release cycle. Patch 3 introduces new helpers. Patches 4 and 5 is just an example how to use new helpers. Locally I have like dozen of them against different subsystems and drivers. Ideally it would go through Input subsystem, thus, needs an Ack from MD maintainer(s). Since v2: - fix compilation issue in MD bitmap code - elaborate changes in commit message of patch 5 Since v1: - added namespace fix patch against MD bitmap API - moved functions to lib/bitmap.c to avoid circular dependencies - appended Dmitry's tags Andy Shevchenko (5): md: Avoid namespace collision with bitmap API bitmap: Drop unnecessary 0 check for u32 array operations bitmap: Add bitmap_alloc(), bitmap_zalloc() and bitmap_free() Input: gpio-keys - Switch to bitmap_zalloc() Input: evdev - Switch to bitmap API drivers/input/evdev.c | 16 +- drivers/input/keyboard/gpio_keys.c| 8 +- drivers/md/dm-raid.c | 6 +- drivers/md/md-bitmap.c| 301 +- drivers/md/md-bitmap.h| 46 +-- drivers/md/md-cluster.c | 16 +- drivers/md/md.c | 44 +-- .../md/persistent-data/dm-space-map-common.c | 12 +- drivers/md/raid1.c| 20 +- drivers/md/raid10.c | 26 +- drivers/md/raid5-cache.c | 2 +- drivers/md/raid5.c| 24 +- include/linux/bitmap.h| 8 + lib/bitmap.c | 28 +- 14 files changed, 283 insertions(+), 274 deletions(-) -- 2.17.1 -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel
[dm-devel] [PATCH v3 4/5] Input: gpio-keys - Switch to bitmap_zalloc()
Switch to bitmap_zalloc() to show clearly what we are allocating. Besides that it returns pointer of bitmap type instead of opaque void *. Acked-by: Dmitry Torokhov Signed-off-by: Andy Shevchenko --- drivers/input/keyboard/gpio_keys.c | 8 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/drivers/input/keyboard/gpio_keys.c b/drivers/input/keyboard/gpio_keys.c index 052e37675086..492a971b95b5 100644 --- a/drivers/input/keyboard/gpio_keys.c +++ b/drivers/input/keyboard/gpio_keys.c @@ -196,7 +196,7 @@ static ssize_t gpio_keys_attr_show_helper(struct gpio_keys_drvdata *ddata, ssize_t ret; int i; - bits = kcalloc(BITS_TO_LONGS(n_events), sizeof(*bits), GFP_KERNEL); + bits = bitmap_zalloc(n_events, GFP_KERNEL); if (!bits) return -ENOMEM; @@ -216,7 +216,7 @@ static ssize_t gpio_keys_attr_show_helper(struct gpio_keys_drvdata *ddata, buf[ret++] = '\n'; buf[ret] = '\0'; - kfree(bits); + bitmap_free(bits); return ret; } @@ -240,7 +240,7 @@ static ssize_t gpio_keys_attr_store_helper(struct gpio_keys_drvdata *ddata, ssize_t error; int i; - bits = kcalloc(BITS_TO_LONGS(n_events), sizeof(*bits), GFP_KERNEL); + bits = bitmap_zalloc(n_events, GFP_KERNEL); if (!bits) return -ENOMEM; @@ -284,7 +284,7 @@ static ssize_t gpio_keys_attr_store_helper(struct gpio_keys_drvdata *ddata, mutex_unlock(&ddata->disable_lock); out: - kfree(bits); + bitmap_free(bits); return error; } -- 2.17.1 -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel
Re: [dm-devel] [PATCH v3 3/5] bitmap: Add bitmap_alloc(), bitmap_zalloc() and bitmap_free()
On Mon, Jun 18, 2018 at 2:14 PM Andrew Morton wrote:
>
> On Mon, 18 Jun 2018 16:10:01 +0300 Andy Shevchenko
> wrote:
>
> > A lot of code become ugly because of open coding allocations for bitmaps.
> >
> > Introduce three helpers to allow users be more clear of intention
> > and keep their code neat.
> >
> > ...
> >
> > +unsigned long *bitmap_alloc(unsigned int nbits, gfp_t flags)
> > +{
> > + return kmalloc_array(BITS_TO_LONGS(nbits), sizeof(unsigned long),
> > flags);
> > +}
> > +EXPORT_SYMBOL(bitmap_alloc);
> > +
> > +unsigned long *bitmap_zalloc(unsigned int nbits, gfp_t flags)
> > +{
> > + return bitmap_alloc(nbits, flags | __GFP_ZERO);
> > +}
> > +EXPORT_SYMBOL(bitmap_zalloc);
> > +
> > +void bitmap_free(const unsigned long *bitmap)
> > +{
> > + kfree(bitmap);
> > +}
> > +EXPORT_SYMBOL(bitmap_free);
> > +
>
> I suggest these functions are small and simple enough to justify
> inlining them.
>
We can't as we end up including bitmap.h (by the way of cpumask.h)
form slab.h, so we gen circular dependency. Maybe if we removed memcg
stuff from slab.h so we do not need to include workqueue.h...
--
Dmitry
--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel
Re: [dm-devel] [PATCH v3 3/5] bitmap: Add bitmap_alloc(), bitmap_zalloc() and bitmap_free()
On Tue, Jun 19, 2018 at 2:10 AM, Andrew Morton
wrote:
> On Mon, 18 Jun 2018 15:01:43 -0700 Dmitry Torokhov
> wrote:
>
>> > > +unsigned long *bitmap_alloc(unsigned int nbits, gfp_t flags)
>> > > +{
>> > > + return kmalloc_array(BITS_TO_LONGS(nbits), sizeof(unsigned long),
>> > > flags);
>> > > +}
>> > > +EXPORT_SYMBOL(bitmap_alloc);
>> > > +
>> > > +unsigned long *bitmap_zalloc(unsigned int nbits, gfp_t flags)
>> > > +{
>> > > + return bitmap_alloc(nbits, flags | __GFP_ZERO);
>> > > +}
>> > > +EXPORT_SYMBOL(bitmap_zalloc);
>> > > +
>> > > +void bitmap_free(const unsigned long *bitmap)
>> > > +{
>> > > + kfree(bitmap);
>> > > +}
>> > > +EXPORT_SYMBOL(bitmap_free);
>> > > +
>> >
>> > I suggest these functions are small and simple enough to justify
>> > inlining them.
>> >
>>
>> We can't as we end up including bitmap.h (by the way of cpumask.h)
>> form slab.h, so we gen circular dependency.
>
> That info should have been in the changelog,
I put it in cover letter, though it perhaps better to have in commit
message itself.
> and probably a code
> comment.
This is done in header file. You meant C-file?
>> Maybe if we removed memcg
>> stuff from slab.h so we do not need to include workqueue.h...
>
> Or move the basic slab API stuff out of slab.h into a new header. Or
> create a new, standalone work_struct.h - that looks pretty simple.
Latter one seems requires least effort without potentially breaking things.
I take it as your suggestion, though I would still give a glance if it
is possible to split exactly memcg part out of slab.
--
With Best Regards,
Andy Shevchenko
--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel
[dm-devel] [PATCH v3 5/5] Input: evdev - Switch to bitmap API
Switch to bitmap API, i.e. bitmap_alloc(), bitmap_zalloc(), to show
clearly what we are allocating. Besides that it returns pointer of
bitmap type instead of opaque void *.
While here, replace memcpy() with bitmap_copy() for sake of consistency.
Acked-by: Dmitry Torokhov
Signed-off-by: Andy Shevchenko
---
drivers/input/evdev.c | 16 +++-
1 file changed, 7 insertions(+), 9 deletions(-)
diff --git a/drivers/input/evdev.c b/drivers/input/evdev.c
index c81c79d01d93..370206f987f9 100644
--- a/drivers/input/evdev.c
+++ b/drivers/input/evdev.c
@@ -481,7 +481,7 @@ static int evdev_release(struct inode *inode, struct file
*file)
evdev_detach_client(evdev, client);
for (i = 0; i < EV_CNT; ++i)
- kfree(client->evmasks[i]);
+ bitmap_free(client->evmasks[i]);
kvfree(client);
@@ -925,17 +925,15 @@ static int evdev_handle_get_val(struct evdev_client
*client,
{
int ret;
unsigned long *mem;
- size_t len;
- len = BITS_TO_LONGS(maxbit) * sizeof(unsigned long);
- mem = kmalloc(len, GFP_KERNEL);
+ mem = bitmap_alloc(maxbit, GFP_KERNEL);
if (!mem)
return -ENOMEM;
spin_lock_irq(&dev->event_lock);
spin_lock(&client->buffer_lock);
- memcpy(mem, bits, len);
+ bitmap_copy(mem, bits, maxbit);
spin_unlock(&dev->event_lock);
@@ -947,7 +945,7 @@ static int evdev_handle_get_val(struct evdev_client *client,
if (ret < 0)
evdev_queue_syn_dropped(client);
- kfree(mem);
+ bitmap_free(mem);
return ret;
}
@@ -1003,13 +1001,13 @@ static int evdev_set_mask(struct evdev_client *client,
if (!cnt)
return 0;
- mask = kcalloc(sizeof(unsigned long), BITS_TO_LONGS(cnt), GFP_KERNEL);
+ mask = bitmap_zalloc(cnt, GFP_KERNEL);
if (!mask)
return -ENOMEM;
error = bits_from_user(mask, cnt - 1, codes_size, codes, compat);
if (error < 0) {
- kfree(mask);
+ bitmap_free(mask);
return error;
}
@@ -1018,7 +1016,7 @@ static int evdev_set_mask(struct evdev_client *client,
client->evmasks[type] = mask;
spin_unlock_irqrestore(&client->buffer_lock, flags);
- kfree(oldmask);
+ bitmap_free(oldmask);
return 0;
}
--
2.17.1
--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel
Re: [dm-devel] [PATCH v2 5/5] Input: evdev - Switch to bitmap_zalloc()
On Sat, 2018-06-16 at 12:16 -0700, Joe Perches wrote: > On Sat, 2018-06-16 at 21:45 +0300, Andy Shevchenko wrote: > > On Sat, Jun 16, 2018 at 12:46 AM Yury Norov > om> wrote: > > > On Fri, Jun 15, 2018 at 04:20:17PM +0300, Andy Shevchenko wrote: > > > > Switch to bitmap_zalloc() to show clearly what we are > > > > allocating. > > > > Besides that it returns pointer of bitmap type instead of opaque > > > > void *. > > > > > > > > + mem = bitmap_alloc(maxbit, GFP_KERNEL); > > > > if (!mem) > > > > return -ENOMEM; > > > > > > But in commit message you say you switch to bitmap_zalloc(). IIUC > > > bitmap_alloc() is OK here. But could you please update comment to > > > avoid confusing. > > > > There are two places, one with alloc, another with zalloc. > > I will clarify this in commit message of next version. > > > > > > + mask = bitmap_zalloc(cnt, GFP_KERNEL); > > > > if (!mask) > > > > return -ENOMEM; > > > > > > > > error = bits_from_user(mask, cnt - 1, codes_size, codes, > > > > compat); > > > > > > If my understanding of bits_from_user() correct, here you can also > > > use > > > bitmap_alloc(), true? > > Also it might be useful to have a separate bitmap_from_user > to alloc and copy. Maybe. I didn't check if there are such users except this driver. Anyway, it's out of scope of the series. -- Andy Shevchenko Intel Finland Oy -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel
Re: [dm-devel] [PATCH v3 1/5] md: Avoid namespace collision with bitmap API
On Mon, 2018-06-18 at 09:44 -0400, Mike Snitzer wrote: > On Mon, Jun 18 2018 at 9:09am -0400, > Andy Shevchenko wrote: > > > bitmap API (include/linux/bitmap.h) has 'bitmap' prefix for its > > methods. > > > > On the other hand MD bitmap API is special case. > > Adding 'md' prefix to it to avoid name space collision. > > > > No functional changes intended. > > > > Signed-off-by: Andy Shevchenko > > --- > > drivers/md/dm-raid.c | 6 +- > > drivers/md/md-bitmap.c| 301 + > > - > > drivers/md/md-bitmap.h| 46 +-- > > drivers/md/md-cluster.c | 16 +- > > drivers/md/md.c | 44 +-- > > .../md/persistent-data/dm-space-map-common.c | 12 +- > > drivers/md/raid1.c| 20 +- > > drivers/md/raid10.c | 26 +- > > drivers/md/raid5-cache.c | 2 +- > > drivers/md/raid5.c| 24 +- > > 10 files changed, 244 insertions(+), 253 deletions(-) > > Seems my previous mail just missed your v3 cutoff but... > > > > All of these drivers/md/persistent-data/dm-space-map-common.c renames > are wrong. So, let me understand this clear, the naming problem appears in one file only. The rest is fine. Correct? > DM is the only consumer (it is confusing because all DM and > MD code lives in drivers/md/ despite only sharing a bit of MD code). > > Anyway, please rename bitmap methods in this file to have a > "dm_bitmap" > prefix instead. Thanks. Ah, okay, will do this in v4 later this week. Thanks for review! -- Andy Shevchenko Intel Finland Oy -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel
Re: [dm-devel] [PATCH v3 3/5] bitmap: Add bitmap_alloc(), bitmap_zalloc() and bitmap_free()
On Tue, Jun 19, 2018 at 1:01 AM, Dmitry Torokhov
wrote:
> On Mon, Jun 18, 2018 at 2:14 PM Andrew Morton
> wrote:
>>
>> On Mon, 18 Jun 2018 16:10:01 +0300 Andy Shevchenko
>> wrote:
>>
>> > A lot of code become ugly because of open coding allocations for bitmaps.
>> >
>> > Introduce three helpers to allow users be more clear of intention
>> > and keep their code neat.
>> >
>> > ...
>> >
>> > +unsigned long *bitmap_alloc(unsigned int nbits, gfp_t flags)
>> > +{
>> > + return kmalloc_array(BITS_TO_LONGS(nbits), sizeof(unsigned long),
>> > flags);
>> > +}
>> > +EXPORT_SYMBOL(bitmap_alloc);
>> > +
>> > +unsigned long *bitmap_zalloc(unsigned int nbits, gfp_t flags)
>> > +{
>> > + return bitmap_alloc(nbits, flags | __GFP_ZERO);
>> > +}
>> > +EXPORT_SYMBOL(bitmap_zalloc);
>> > +
>> > +void bitmap_free(const unsigned long *bitmap)
>> > +{
>> > + kfree(bitmap);
>> > +}
>> > +EXPORT_SYMBOL(bitmap_free);
>> > +
>>
>> I suggest these functions are small and simple enough to justify
>> inlining them.
>>
>
> We can't as we end up including bitmap.h (by the way of cpumask.h)
> form slab.h, so we gen circular dependency. Maybe if we removed memcg
> stuff from slab.h so we do not need to include workqueue.h...
I will look at it. It might be doable.
Though I dunno what MM people would say about this. Anyone's name
comes to your mind to ask?
--
With Best Regards,
Andy Shevchenko
--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel
Re: [dm-devel] [PATCH v2 5/5] Input: evdev - Switch to bitmap_zalloc()
On Mon, Jun 18, 2018 at 6:49 PM, Joe Perches wrote: > On Mon, 2018-06-18 at 15:02 +0300, Andy Shevchenko wrote: >> On Sat, 2018-06-16 at 12:16 -0700, Joe Perches wrote: >> > On Sat, 2018-06-16 at 21:45 +0300, Andy Shevchenko wrote: >> > > On Sat, Jun 16, 2018 at 12:46 AM Yury Norov > > > om> wrote: >> > > > On Fri, Jun 15, 2018 at 04:20:17PM +0300, Andy Shevchenko wrote: >> > > > > Switch to bitmap_zalloc() to show clearly what we are >> > > > > allocating. >> > > > > Besides that it returns pointer of bitmap type instead of opaque >> > > > > void *. >> > > >> > > >> > > > > + mem = bitmap_alloc(maxbit, GFP_KERNEL); >> > > > > if (!mem) >> > > > > return -ENOMEM; >> > > > >> > > > But in commit message you say you switch to bitmap_zalloc(). IIUC >> > > > bitmap_alloc() is OK here. But could you please update comment to >> > > > avoid confusing. >> > > >> > > There are two places, one with alloc, another with zalloc. >> > > I will clarify this in commit message of next version. >> > > >> > > > > + mask = bitmap_zalloc(cnt, GFP_KERNEL); >> > > > > if (!mask) >> > > > > return -ENOMEM; >> > > > > >> > > > > error = bits_from_user(mask, cnt - 1, codes_size, codes, >> > > > > compat); >> > > > >> > > > If my understanding of bits_from_user() correct, here you can also >> > > > use >> > > > bitmap_alloc(), true? >> > >> > Also it might be useful to have a separate bitmap_from_user >> > to alloc and copy. >> >> Maybe. I didn't check if there are such users except this driver. >> >> Anyway, it's out of scope of the series. > > That seems incorrect as you are introducing alloc/free helpers. > > Perhaps bitmap_dup_user [or some better name] could or should > be one of the helpers. Can you help with estimation how many existing users need this kind of functionality? One of them evdev, which has an open coded variant. Also, pay attention to that fact we have already bitmap_parse_user() and bitmap_parlelist_user() which are called by several users. If the estimation will show something like 3+, it would definitely make sense, otherwise, I wouldn't like spend time on it. -- With Best Regards, Andy Shevchenko -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel
