Linux RAID subsystem development
 help / color / mirror / Atom feed
* [PATCH 02/12] raid5-cache: free I/O units earlier
From: Christoph Hellwig @ 2015-09-12  6:17 UTC (permalink / raw)
  To: Shaohua Li, neilb; +Cc: linux-raid, Kernel-team, dan.j.williams
In-Reply-To: <1442038638-6947-1-git-send-email-hch@lst.de>

There is no good reason to keep the I/O unit structures around after the
stripe has been written back to the RAID array.  The only information
we need is the log sequence number, and the checkpoint offset of the
highest successfull writeback.  Store those in the log structure, and
free the IO units from __r5l_stripe_write_finished.

Besides simplifying the code this also avoid having to keep the allocation
for the I/O unit around for a potentially long time as superblock updates
that checkpoint the log do not happen very often.

This also fixes the previously incorrect calculation of 'free' in
r5l_do_reclaim as a side effect: previous if took the last unit which
isn't checkpointed into account.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 drivers/md/raid5-cache.c | 145 ++++++++++++++++++-----------------------------
 1 file changed, 55 insertions(+), 90 deletions(-)

diff --git a/drivers/md/raid5-cache.c b/drivers/md/raid5-cache.c
index c345479..803bcc6 100644
--- a/drivers/md/raid5-cache.c
+++ b/drivers/md/raid5-cache.c
@@ -51,6 +51,9 @@ struct r5l_log {
 	sector_t log_start; /* log head. where new data appends */
 	u64 seq; /* log head sequence */
 
+	sector_t next_checkpoint;
+	u64 next_cp_seq;
+
 	struct mutex io_mutex;
 	struct r5l_io_unit *current_io; /* current io_unit accepting new data */
 
@@ -65,10 +68,6 @@ struct r5l_log {
 					* cache flush */
 	struct list_head flushed_ios; /* io_units which settle down in log disk */
 	struct bio flush_bio;
-	struct list_head stripe_end_ios; /* io_units which have been
-					  * completely written to the RAID *
-					  * but have not yet been considered *
-					  * for updating super */
 
 	struct kmem_cache *io_kc;
 
@@ -185,35 +184,6 @@ static void r5l_move_io_unit_list(struct list_head *from, struct list_head *to,
        }
 }
 
-/*
- * We don't want too many io_units reside in stripe_end_ios list, which will
- * waste a lot of memory. So we try to remove some. But we must keep at least 2
- * io_units. The superblock must point to a valid meta, if it's the last meta,
- * recovery can scan less
- * */
-static void r5l_compress_stripe_end_list(struct r5l_log *log)
-{
-	struct r5l_io_unit *first, *last, *io;
-
-	first = list_first_entry(&log->stripe_end_ios,
-		struct r5l_io_unit, log_sibling);
-	last = list_last_entry(&log->stripe_end_ios,
-		struct r5l_io_unit, log_sibling);
-	if (first == last)
-		return;
-	list_del(&first->log_sibling);
-	list_del(&last->log_sibling);
-	while (!list_empty(&log->stripe_end_ios)) {
-		io = list_first_entry(&log->stripe_end_ios,
-			struct r5l_io_unit, log_sibling);
-		list_del(&io->log_sibling);
-		first->log_end = io->log_end;
-		r5l_free_io_unit(log, io);
-	}
-	list_add_tail(&first->log_sibling, &log->stripe_end_ios);
-	list_add_tail(&last->log_sibling, &log->stripe_end_ios);
-}
-
 static void __r5l_set_io_unit_state(struct r5l_io_unit *io,
 	enum r5l_io_unit_state state)
 {
@@ -540,31 +510,53 @@ static void r5l_run_no_space_stripes(struct r5l_log *log)
 	spin_unlock(&log->no_space_stripes_lock);
 }
 
+static sector_t r5l_reclaimable_space(struct r5l_log *log)
+{
+	return r5l_ring_distance(log, log->last_checkpoint,
+				 log->next_checkpoint);
+}
+
+static bool r5l_complete_flushed_ios(struct r5l_log *log)
+{
+	struct r5l_io_unit *io, *next;
+	bool found = false;
+
+	assert_spin_locked(&log->io_list_lock);
+
+	list_for_each_entry_safe(io, next, &log->flushed_ios, log_sibling) {
+		/* don't change list order */
+		if (io->state < IO_UNIT_STRIPE_END)
+			break;
+
+		log->next_checkpoint = io->log_start;
+		log->next_cp_seq = io->seq;
+
+		list_del(&io->log_sibling);
+		r5l_free_io_unit(log, io);
+
+		found = true;
+	}
+	
+
+	return found;
+}
+
 static void __r5l_stripe_write_finished(struct r5l_io_unit *io)
 {
 	struct r5l_log *log = io->log;
-	struct r5l_io_unit *last;
-	sector_t reclaimable_space;
 	unsigned long flags;
 
 	spin_lock_irqsave(&log->io_list_lock, flags);
 	__r5l_set_io_unit_state(io, IO_UNIT_STRIPE_END);
-	/* might move 0 entry */
-	r5l_move_io_unit_list(&log->flushed_ios, &log->stripe_end_ios,
-		IO_UNIT_STRIPE_END);
-	if (list_empty(&log->stripe_end_ios)) {
+
+	if (!r5l_complete_flushed_ios(log)) {
 		spin_unlock_irqrestore(&log->io_list_lock, flags);
 		return;
 	}
 
-	last = list_last_entry(&log->stripe_end_ios,
-			struct r5l_io_unit, log_sibling);
-	reclaimable_space = r5l_ring_distance(log, log->last_checkpoint,
-				last->log_end);
-	if (reclaimable_space >= log->max_free_space)
+	if (r5l_reclaimable_space(log) > log->max_free_space)
 		r5l_wake_reclaim(log, 0);
 
-	r5l_compress_stripe_end_list(log);
 	spin_unlock_irqrestore(&log->io_list_lock, flags);
 	wake_up(&log->iounit_wait);
 }
@@ -640,13 +632,6 @@ void r5l_flush_stripe_to_raid(struct r5l_log *log)
 	submit_bio(WRITE_FLUSH, &log->flush_bio);
 }
 
-static void r5l_kick_io_unit(struct r5l_log *log)
-{
-	md_wakeup_thread(log->rdev->mddev->thread);
-	wait_event_lock_irq(log->iounit_wait, !list_empty(&log->stripe_end_ios),
-		log->io_list_lock);
-}
-
 static void r5l_write_super(struct r5l_log *log, sector_t cp);
 static void r5l_write_super_and_discard_space(struct r5l_log *log,
 	sector_t end)
@@ -678,10 +663,10 @@ static void r5l_write_super_and_discard_space(struct r5l_log *log,
 
 static void r5l_do_reclaim(struct r5l_log *log)
 {
-	struct r5l_io_unit *io, *last;
-	LIST_HEAD(list);
-	sector_t free = 0;
 	sector_t reclaim_target = xchg(&log->reclaim_target, 0);
+	sector_t reclaimable;
+	sector_t next_checkpoint;
+	u64 next_cp_seq;
 
 	spin_lock_irq(&log->io_list_lock);
 	/*
@@ -690,60 +675,41 @@ static void r5l_do_reclaim(struct r5l_log *log)
 	 * shouldn't reuse space of an unreclaimable io_unit
 	 * */
 	while (1) {
-		struct list_head *target_list = NULL;
-
-		while (!list_empty(&log->stripe_end_ios)) {
-			io = list_first_entry(&log->stripe_end_ios,
-				struct r5l_io_unit, log_sibling);
-			list_move_tail(&io->log_sibling, &list);
-			free += r5l_ring_distance(log, io->log_start,
-						io->log_end);
-		}
-
-		if (free >= reclaim_target ||
+		reclaimable = r5l_reclaimable_space(log);
+		if (reclaimable >= reclaim_target ||
 		    (list_empty(&log->running_ios) &&
 		     list_empty(&log->io_end_ios) &&
 		     list_empty(&log->flushing_ios) &&
 		     list_empty(&log->flushed_ios)))
 			break;
 
-		/* Below waiting mostly happens when we shutdown the raid */
-		if (!list_empty(&log->flushed_ios))
-			target_list = &log->flushed_ios;
-		else if (!list_empty(&log->flushing_ios))
-			target_list = &log->flushing_ios;
-		else if (!list_empty(&log->io_end_ios))
-			target_list = &log->io_end_ios;
-		else if (!list_empty(&log->running_ios))
-			target_list = &log->running_ios;
-
-		r5l_kick_io_unit(log);
+		md_wakeup_thread(log->rdev->mddev->thread);
+		wait_event_lock_irq(log->iounit_wait,
+				    r5l_reclaimable_space(log) > reclaimable,
+				    log->io_list_lock);
 	}
+
+	next_checkpoint = log->next_checkpoint;
+	next_cp_seq = log->next_cp_seq;
 	spin_unlock_irq(&log->io_list_lock);
 
-	if (list_empty(&list))
+	BUG_ON(reclaimable < 0);
+	if (reclaimable == 0)
 		return;
 
-	/* super always point to last valid meta */
-	last = list_last_entry(&list, struct r5l_io_unit, log_sibling);
 	/*
 	 * write_super will flush cache of each raid disk. We must write super
 	 * here, because the log area might be reused soon and we don't want to
 	 * confuse recovery
 	 * */
-	r5l_write_super_and_discard_space(log, last->log_start);
+	r5l_write_super_and_discard_space(log, next_checkpoint);
 
 	mutex_lock(&log->io_mutex);
-	log->last_checkpoint = last->log_start;
-	log->last_cp_seq = last->seq;
+	log->last_checkpoint = next_checkpoint;
+	log->last_cp_seq = next_cp_seq;
 	mutex_unlock(&log->io_mutex);
-	r5l_run_no_space_stripes(log);
 
-	while (!list_empty(&list)) {
-		io = list_first_entry(&list, struct r5l_io_unit, log_sibling);
-		list_del(&io->log_sibling);
-		r5l_free_io_unit(log, io);
-	}
+	r5l_run_no_space_stripes(log);
 }
 
 static void r5l_reclaim_thread(struct md_thread *thread)
@@ -1105,7 +1071,6 @@ int r5l_init_log(struct r5conf *conf, struct md_rdev *rdev)
 	spin_lock_init(&log->io_list_lock);
 	INIT_LIST_HEAD(&log->running_ios);
 	INIT_LIST_HEAD(&log->io_end_ios);
-	INIT_LIST_HEAD(&log->stripe_end_ios);
 	INIT_LIST_HEAD(&log->flushing_ios);
 	INIT_LIST_HEAD(&log->flushed_ios);
 	bio_init(&log->flush_bio);
-- 
1.9.1


^ permalink raw reply related

* [PATCH 03/12] raid5-cache: rename flushed_ios to finished_ios
From: Christoph Hellwig @ 2015-09-12  6:17 UTC (permalink / raw)
  To: Shaohua Li, neilb; +Cc: linux-raid, Kernel-team, dan.j.williams
In-Reply-To: <1442038638-6947-1-git-send-email-hch@lst.de>

After adding FUA support we won't nessecarily have called flushed for the
bios on this list, so give it a more neutral name.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 drivers/md/raid5-cache.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/drivers/md/raid5-cache.c b/drivers/md/raid5-cache.c
index 803bcc6..4442dd3 100644
--- a/drivers/md/raid5-cache.c
+++ b/drivers/md/raid5-cache.c
@@ -66,7 +66,7 @@ struct r5l_log {
 				      * to the RAID */
 	struct list_head flushing_ios; /* io_units which are waiting for log
 					* cache flush */
-	struct list_head flushed_ios; /* io_units which settle down in log disk */
+	struct list_head finished_ios; /* io_units which settle down in log disk */
 	struct bio flush_bio;
 
 	struct kmem_cache *io_kc;
@@ -516,14 +516,14 @@ static sector_t r5l_reclaimable_space(struct r5l_log *log)
 				 log->next_checkpoint);
 }
 
-static bool r5l_complete_flushed_ios(struct r5l_log *log)
+static bool r5l_complete_finished_ios(struct r5l_log *log)
 {
 	struct r5l_io_unit *io, *next;
 	bool found = false;
 
 	assert_spin_locked(&log->io_list_lock);
 
-	list_for_each_entry_safe(io, next, &log->flushed_ios, log_sibling) {
+	list_for_each_entry_safe(io, next, &log->finished_ios, log_sibling) {
 		/* don't change list order */
 		if (io->state < IO_UNIT_STRIPE_END)
 			break;
@@ -549,7 +549,7 @@ static void __r5l_stripe_write_finished(struct r5l_io_unit *io)
 	spin_lock_irqsave(&log->io_list_lock, flags);
 	__r5l_set_io_unit_state(io, IO_UNIT_STRIPE_END);
 
-	if (!r5l_complete_flushed_ios(log)) {
+	if (!r5l_complete_finished_ios(log)) {
 		spin_unlock_irqrestore(&log->io_list_lock, flags);
 		return;
 	}
@@ -590,7 +590,7 @@ static void r5l_log_flush_endio(struct bio *bio)
 			raid5_release_stripe(sh);
 		}
 	}
-	list_splice_tail_init(&log->flushing_ios, &log->flushed_ios);
+	list_splice_tail_init(&log->flushing_ios, &log->finished_ios);
 	spin_unlock_irqrestore(&log->io_list_lock, flags);
 }
 
@@ -680,7 +680,7 @@ static void r5l_do_reclaim(struct r5l_log *log)
 		    (list_empty(&log->running_ios) &&
 		     list_empty(&log->io_end_ios) &&
 		     list_empty(&log->flushing_ios) &&
-		     list_empty(&log->flushed_ios)))
+		     list_empty(&log->finished_ios)))
 			break;
 
 		md_wakeup_thread(log->rdev->mddev->thread);
@@ -1072,7 +1072,7 @@ int r5l_init_log(struct r5conf *conf, struct md_rdev *rdev)
 	INIT_LIST_HEAD(&log->running_ios);
 	INIT_LIST_HEAD(&log->io_end_ios);
 	INIT_LIST_HEAD(&log->flushing_ios);
-	INIT_LIST_HEAD(&log->flushed_ios);
+	INIT_LIST_HEAD(&log->finished_ios);
 	bio_init(&log->flush_bio);
 
 	log->io_kc = KMEM_CACHE(r5l_io_unit, 0);
-- 
1.9.1


^ permalink raw reply related

* [PATCH 04/12] raid5-cache: factor out a helper to run all stripes for an I/O unit
From: Christoph Hellwig @ 2015-09-12  6:17 UTC (permalink / raw)
  To: Shaohua Li, neilb; +Cc: linux-raid, Kernel-team, dan.j.williams
In-Reply-To: <1442038638-6947-1-git-send-email-hch@lst.de>

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 drivers/md/raid5-cache.c | 23 +++++++++++++----------
 1 file changed, 13 insertions(+), 10 deletions(-)

diff --git a/drivers/md/raid5-cache.c b/drivers/md/raid5-cache.c
index 4442dd3..c288c43 100644
--- a/drivers/md/raid5-cache.c
+++ b/drivers/md/raid5-cache.c
@@ -192,6 +192,17 @@ static void __r5l_set_io_unit_state(struct r5l_io_unit *io,
 	io->state = state;
 }
 
+static void r5l_io_run_stripes(struct r5l_io_unit *io)
+{
+	struct stripe_head *sh, *next;
+
+	list_for_each_entry_safe(sh, next, &io->stripe_list, log_list) {
+		list_del_init(&sh->log_list);
+		set_bit(STRIPE_HANDLE, &sh->state);
+		raid5_release_stripe(sh);
+	}
+}
+
 /* XXX: totally ignores I/O errors */
 static void r5l_log_endio(struct bio *bio)
 {
@@ -578,18 +589,10 @@ static void r5l_log_flush_endio(struct bio *bio)
 		flush_bio);
 	unsigned long flags;
 	struct r5l_io_unit *io;
-	struct stripe_head *sh;
 
 	spin_lock_irqsave(&log->io_list_lock, flags);
-	list_for_each_entry(io, &log->flushing_ios, log_sibling) {
-		while (!list_empty(&io->stripe_list)) {
-			sh = list_first_entry(&io->stripe_list,
-				struct stripe_head, log_list);
-			list_del_init(&sh->log_list);
-			set_bit(STRIPE_HANDLE, &sh->state);
-			raid5_release_stripe(sh);
-		}
-	}
+	list_for_each_entry(io, &log->flushing_ios, log_sibling)
+		r5l_io_run_stripes(io);
 	list_splice_tail_init(&log->flushing_ios, &log->finished_ios);
 	spin_unlock_irqrestore(&log->io_list_lock, flags);
 }
-- 
1.9.1


^ permalink raw reply related

* [PATCH 05/12] raid5-cache: use FUA writes for the log
From: Christoph Hellwig @ 2015-09-12  6:17 UTC (permalink / raw)
  To: Shaohua Li, neilb; +Cc: linux-raid, Kernel-team, dan.j.williams
In-Reply-To: <1442038638-6947-1-git-send-email-hch@lst.de>

If we submit writes with the FUA bit for the log they are guaranteed to
be on stable storage once the endio callback is called.  This allows
to simplify the IO unit state machine, and decrease latencies a lot
when the device supports FUA.  Use this to improve performance on devices
that support FUA, or keep the old code if the device doesn't support it.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 drivers/md/raid5-cache.c | 35 ++++++++++++++++++++++++++++++-----
 1 file changed, 30 insertions(+), 5 deletions(-)

diff --git a/drivers/md/raid5-cache.c b/drivers/md/raid5-cache.c
index c288c43..d813c89 100644
--- a/drivers/md/raid5-cache.c
+++ b/drivers/md/raid5-cache.c
@@ -81,6 +81,8 @@ struct r5l_log {
 
 	struct list_head no_space_stripes; /* pending stripes, log has no space */
 	spinlock_t no_space_stripes_lock;
+
+	bool use_fua;
 };
 
 /*
@@ -203,6 +205,22 @@ static void r5l_io_run_stripes(struct r5l_io_unit *io)
 	}
 }
 
+static void r5l_log_run_stripes(struct r5l_log *log)
+{
+	struct r5l_io_unit *io, *next;
+
+	assert_spin_locked(&log->io_list_lock);
+
+	list_for_each_entry_safe(io, next, &log->running_ios, log_sibling) {
+		/* don't change list order */
+		if (io->state < IO_UNIT_IO_END)
+			break;
+
+		list_move_tail(&io->log_sibling, &log->finished_ios);
+		r5l_io_run_stripes(io);
+	}
+}
+
 /* XXX: totally ignores I/O errors */
 static void r5l_log_endio(struct bio *bio)
 {
@@ -217,11 +235,15 @@ static void r5l_log_endio(struct bio *bio)
 
 	spin_lock_irqsave(&log->io_list_lock, flags);
 	__r5l_set_io_unit_state(io, IO_UNIT_IO_END);
-	r5l_move_io_unit_list(&log->running_ios, &log->io_end_ios,
-			IO_UNIT_IO_END);
+	if (log->use_fua)
+		r5l_log_run_stripes(log);
+	else
+		r5l_move_io_unit_list(&log->running_ios, &log->io_end_ios,
+				      IO_UNIT_IO_END);
 	spin_unlock_irqrestore(&log->io_list_lock, flags);
 
-	md_wakeup_thread(log->rdev->mddev->thread);
+	if (!log->use_fua)
+		md_wakeup_thread(log->rdev->mddev->thread);
 }
 
 static void r5l_submit_current_io(struct r5l_log *log)
@@ -248,7 +270,7 @@ static void r5l_submit_current_io(struct r5l_log *log)
 	while ((bio = bio_list_pop(&io->bios))) {
 		/* all IO must start from rdev->data_offset */
 		bio->bi_iter.bi_sector += log->rdev->data_offset;
-		submit_bio(WRITE, bio);
+		submit_bio(WRITE | (log->use_fua ? REQ_FUA : 0), bio);
 	}
 }
 
@@ -614,7 +636,8 @@ static void r5l_log_flush_endio(struct bio *bio)
 void r5l_flush_stripe_to_raid(struct r5l_log *log)
 {
 	bool do_flush;
-	if (!log)
+
+	if (!log || log->use_fua)
 		return;
 
 	spin_lock_irq(&log->io_list_lock);
@@ -1066,6 +1089,8 @@ int r5l_init_log(struct r5conf *conf, struct md_rdev *rdev)
 		return -ENOMEM;
 	log->rdev = rdev;
 
+	log->use_fua = (rdev->bdev->bd_disk->queue->flush_flags & REQ_FUA);
+
 	log->uuid_checksum = crc32_le(~0, (void *)rdev->mddev->uuid,
 			sizeof(rdev->mddev->uuid));
 
-- 
1.9.1


^ permalink raw reply related

* [PATCH 06/12] raid5-cache: clean up r5l_get_meta
From: Christoph Hellwig @ 2015-09-12  6:17 UTC (permalink / raw)
  To: Shaohua Li, neilb; +Cc: linux-raid, Kernel-team, dan.j.williams
In-Reply-To: <1442038638-6947-1-git-send-email-hch@lst.de>

Remove the only partially used local 'io' variable to simplify the code
flow.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 drivers/md/raid5-cache.c | 12 ++++--------
 1 file changed, 4 insertions(+), 8 deletions(-)

diff --git a/drivers/md/raid5-cache.c b/drivers/md/raid5-cache.c
index d813c89..f9aaf2b 100644
--- a/drivers/md/raid5-cache.c
+++ b/drivers/md/raid5-cache.c
@@ -320,16 +320,12 @@ static struct r5l_io_unit *r5l_new_meta(struct r5l_log *log)
 
 static int r5l_get_meta(struct r5l_log *log, unsigned int payload_size)
 {
-	struct r5l_io_unit *io;
-
-	io = log->current_io;
-	if (io && io->meta_offset + payload_size > PAGE_SIZE)
+	if (log->current_io &&
+	    log->current_io->meta_offset + payload_size > PAGE_SIZE)
 		r5l_submit_current_io(log);
-	io = log->current_io;
-	if (io)
-		return 0;
 
-	log->current_io = r5l_new_meta(log);
+	if (!log->current_io)
+		log->current_io = r5l_new_meta(log);
 	return 0;
 }
 
-- 
1.9.1


^ permalink raw reply related

* [PATCH 07/12] raid5-cache: refactor bio allocation
From: Christoph Hellwig @ 2015-09-12  6:17 UTC (permalink / raw)
  To: Shaohua Li, neilb; +Cc: linux-raid, Kernel-team, dan.j.williams
In-Reply-To: <1442038638-6947-1-git-send-email-hch@lst.de>

Split out a helper to allocate a bio for log writes.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 drivers/md/raid5-cache.c | 44 ++++++++++++++++++++------------------------
 1 file changed, 20 insertions(+), 24 deletions(-)

diff --git a/drivers/md/raid5-cache.c b/drivers/md/raid5-cache.c
index f9aaf2b..fb1ac4c 100644
--- a/drivers/md/raid5-cache.c
+++ b/drivers/md/raid5-cache.c
@@ -274,11 +274,25 @@ static void r5l_submit_current_io(struct r5l_log *log)
 	}
 }
 
+static struct bio *r5l_bio_alloc(struct r5l_log *log, struct r5l_io_unit *io)
+{
+	struct bio *bio = bio_kmalloc(GFP_NOIO | __GFP_NOFAIL, BIO_MAX_PAGES);
+
+	bio->bi_rw = WRITE;
+	bio->bi_bdev = log->rdev->bdev;
+	bio->bi_iter.bi_sector = log->log_start;
+	bio->bi_end_io = r5l_log_endio;
+	bio->bi_private = io;
+
+	bio_list_add(&io->bios, bio);
+	atomic_inc(&io->pending_io);
+	return bio;
+}
+
 static struct r5l_io_unit *r5l_new_meta(struct r5l_log *log)
 {
 	struct r5l_io_unit *io;
 	struct r5l_meta_block *block;
-	struct bio *bio;
 
 	io = r5l_alloc_io_unit(log);
 
@@ -292,17 +306,8 @@ static struct r5l_io_unit *r5l_new_meta(struct r5l_log *log)
 	io->meta_offset = sizeof(struct r5l_meta_block);
 	io->seq = log->seq;
 
-	bio = bio_kmalloc(GFP_NOIO | __GFP_NOFAIL, BIO_MAX_PAGES);
-	io->current_bio = bio;
-	bio->bi_rw = WRITE;
-	bio->bi_bdev = log->rdev->bdev;
-	bio->bi_iter.bi_sector = log->log_start;
-	bio_add_page(bio, io->meta_page, PAGE_SIZE, 0);
-	bio->bi_end_io = r5l_log_endio;
-	bio->bi_private = io;
-
-	bio_list_add(&io->bios, bio);
-	atomic_inc(&io->pending_io);
+	io->current_bio = r5l_bio_alloc(log, io);
+	bio_add_page(io->current_bio, io->meta_page, PAGE_SIZE, 0);
 
 	log->seq++;
 	log->log_start = r5l_ring_add(log, log->log_start, BLOCK_SECTORS);
@@ -355,18 +360,9 @@ static void r5l_append_payload_page(struct r5l_log *log, struct page *page)
 	struct r5l_io_unit *io = log->current_io;
 
 alloc_bio:
-	if (!io->current_bio) {
-		struct bio *bio;
-		bio = bio_kmalloc(GFP_NOIO | __GFP_NOFAIL, BIO_MAX_PAGES);
-		bio->bi_rw = WRITE;
-		bio->bi_bdev = log->rdev->bdev;
-		bio->bi_iter.bi_sector = log->log_start;
-		bio->bi_end_io = r5l_log_endio;
-		bio->bi_private = io;
-		bio_list_add(&io->bios, bio);
-		atomic_inc(&io->pending_io);
-		io->current_bio = bio;
-	}
+	if (!io->current_bio) 
+		io->current_bio = r5l_bio_alloc(log, io);
+
 	if (!bio_add_page(io->current_bio, page, PAGE_SIZE, 0)) {
 		io->current_bio = NULL;
 		goto alloc_bio;
-- 
1.9.1


^ permalink raw reply related

* [PATCH 08/12] raid5-cache: take rdev->data_offset into account early on
From: Christoph Hellwig @ 2015-09-12  6:17 UTC (permalink / raw)
  To: Shaohua Li, neilb; +Cc: linux-raid, Kernel-team, dan.j.williams
In-Reply-To: <1442038638-6947-1-git-send-email-hch@lst.de>

Set up bi_sector properly when we allocate an bio instead of updating it
at submission time.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 drivers/md/raid5-cache.c | 7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/drivers/md/raid5-cache.c b/drivers/md/raid5-cache.c
index fb1ac4c..df24e9c 100644
--- a/drivers/md/raid5-cache.c
+++ b/drivers/md/raid5-cache.c
@@ -267,11 +267,8 @@ static void r5l_submit_current_io(struct r5l_log *log)
 	__r5l_set_io_unit_state(io, IO_UNIT_IO_START);
 	spin_unlock_irqrestore(&log->io_list_lock, flags);
 
-	while ((bio = bio_list_pop(&io->bios))) {
-		/* all IO must start from rdev->data_offset */
-		bio->bi_iter.bi_sector += log->rdev->data_offset;
+	while ((bio = bio_list_pop(&io->bios)))
 		submit_bio(WRITE | (log->use_fua ? REQ_FUA : 0), bio);
-	}
 }
 
 static struct bio *r5l_bio_alloc(struct r5l_log *log, struct r5l_io_unit *io)
@@ -280,7 +277,7 @@ static struct bio *r5l_bio_alloc(struct r5l_log *log, struct r5l_io_unit *io)
 
 	bio->bi_rw = WRITE;
 	bio->bi_bdev = log->rdev->bdev;
-	bio->bi_iter.bi_sector = log->log_start;
+	bio->bi_iter.bi_sector = log->rdev->data_offset + log->log_start;
 	bio->bi_end_io = r5l_log_endio;
 	bio->bi_private = io;
 
-- 
1.9.1


^ permalink raw reply related

* [PATCH 09/12] raid5-cache: inline r5l_alloc_io_unit into r5l_new_meta
From: Christoph Hellwig @ 2015-09-12  6:17 UTC (permalink / raw)
  To: Shaohua Li, neilb; +Cc: linux-raid, Kernel-team, dan.j.williams
In-Reply-To: <1442038638-6947-1-git-send-email-hch@lst.de>

This is the only user, and keeping all code initializing the io_unit
structure together improves readbility.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 drivers/md/raid5-cache.c | 26 ++++++++------------------
 1 file changed, 8 insertions(+), 18 deletions(-)

diff --git a/drivers/md/raid5-cache.c b/drivers/md/raid5-cache.c
index df24e9c..82a9d32 100644
--- a/drivers/md/raid5-cache.c
+++ b/drivers/md/raid5-cache.c
@@ -148,23 +148,6 @@ static bool r5l_has_free_space(struct r5l_log *log, sector_t size)
 	return log->device_size > used_size + size;
 }
 
-static struct r5l_io_unit *r5l_alloc_io_unit(struct r5l_log *log)
-{
-	struct r5l_io_unit *io;
-	/* We can't handle memory allocate failure so far */
-	gfp_t gfp = GFP_NOIO | __GFP_NOFAIL;
-
-	io = kmem_cache_zalloc(log->io_kc, gfp);
-	io->log = log;
-	io->meta_page = alloc_page(gfp | __GFP_ZERO);
-
-	bio_list_init(&io->bios);
-	INIT_LIST_HEAD(&io->log_sibling);
-	INIT_LIST_HEAD(&io->stripe_list);
-	io->state = IO_UNIT_RUNNING;
-	return io;
-}
-
 static void r5l_free_io_unit(struct r5l_log *log, struct r5l_io_unit *io)
 {
 	__free_page(io->meta_page);
@@ -291,8 +274,15 @@ static struct r5l_io_unit *r5l_new_meta(struct r5l_log *log)
 	struct r5l_io_unit *io;
 	struct r5l_meta_block *block;
 
-	io = r5l_alloc_io_unit(log);
+	/* We can't handle memory allocate failure so far */
+	io = kmem_cache_zalloc(log->io_kc, GFP_NOIO | __GFP_NOFAIL);
+	io->log = log;
+	bio_list_init(&io->bios);
+	INIT_LIST_HEAD(&io->log_sibling);
+	INIT_LIST_HEAD(&io->stripe_list);
+	io->state = IO_UNIT_RUNNING;
 
+	io->meta_page = alloc_page(GFP_NOIO | __GFP_NOFAIL | __GFP_ZERO);
 	block = page_address(io->meta_page);
 	block->magic = cpu_to_le32(R5LOG_MAGIC);
 	block->version = R5LOG_VERSION;
-- 
1.9.1


^ permalink raw reply related

* [PATCH 10/12] raid5-cache: new helper: r5_reserve_log_entry
From: Christoph Hellwig @ 2015-09-12  6:17 UTC (permalink / raw)
  To: Shaohua Li, neilb; +Cc: linux-raid, Kernel-team, dan.j.williams
In-Reply-To: <1442038638-6947-1-git-send-email-hch@lst.de>

Factor out code to reserve log space.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 drivers/md/raid5-cache.c | 30 +++++++++++++++++++-----------
 1 file changed, 19 insertions(+), 11 deletions(-)

diff --git a/drivers/md/raid5-cache.c b/drivers/md/raid5-cache.c
index 82a9d32..7d9fa29 100644
--- a/drivers/md/raid5-cache.c
+++ b/drivers/md/raid5-cache.c
@@ -269,6 +269,23 @@ static struct bio *r5l_bio_alloc(struct r5l_log *log, struct r5l_io_unit *io)
 	return bio;
 }
 
+static void r5_reserve_log_entry(struct r5l_log *log, struct r5l_io_unit *io)
+{
+	log->log_start = r5l_ring_add(log, log->log_start, BLOCK_SECTORS);
+
+	/*
+	 * If we filled up the log device start from the beginning again,
+	 * which will require a new bio.
+	 *
+	 * Note: for this to work properly the log size needs to me a multiple
+	 * of BLOCK_SECTORS.
+	 */
+	if (log->log_start == 0)
+		io->current_bio = NULL;
+
+	io->log_end = log->log_start;
+}
+
 static struct r5l_io_unit *r5l_new_meta(struct r5l_log *log)
 {
 	struct r5l_io_unit *io;
@@ -297,11 +314,7 @@ static struct r5l_io_unit *r5l_new_meta(struct r5l_log *log)
 	bio_add_page(io->current_bio, io->meta_page, PAGE_SIZE, 0);
 
 	log->seq++;
-	log->log_start = r5l_ring_add(log, log->log_start, BLOCK_SECTORS);
-	io->log_end = log->log_start;
-	/* current bio hit disk end */
-	if (log->log_start == 0)
-		io->current_bio = NULL;
+	r5_reserve_log_entry(log, io);
 
 	spin_lock_irq(&log->io_list_lock);
 	list_add_tail(&io->log_sibling, &log->running_ios);
@@ -354,13 +367,8 @@ alloc_bio:
 		io->current_bio = NULL;
 		goto alloc_bio;
 	}
-	log->log_start = r5l_ring_add(log, log->log_start,
-			BLOCK_SECTORS);
-	/* current bio hit disk end */
-	if (log->log_start == 0)
-		io->current_bio = NULL;
 
-	io->log_end = log->log_start;
+	r5_reserve_log_entry(log, io);
 }
 
 static void r5l_log_stripe(struct r5l_log *log, struct stripe_head *sh,
-- 
1.9.1


^ permalink raw reply related

* [PATCH 11/12] raid5-cache: small log->seq cleanup
From: Christoph Hellwig @ 2015-09-12  6:17 UTC (permalink / raw)
  To: Shaohua Li, neilb; +Cc: linux-raid, Kernel-team, dan.j.williams
In-Reply-To: <1442038638-6947-1-git-send-email-hch@lst.de>

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 drivers/md/raid5-cache.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/md/raid5-cache.c b/drivers/md/raid5-cache.c
index 7d9fa29..858bfcf 100644
--- a/drivers/md/raid5-cache.c
+++ b/drivers/md/raid5-cache.c
@@ -308,12 +308,11 @@ static struct r5l_io_unit *r5l_new_meta(struct r5l_log *log)
 
 	io->log_start = log->log_start;
 	io->meta_offset = sizeof(struct r5l_meta_block);
-	io->seq = log->seq;
+	io->seq = log->seq++;
 
 	io->current_bio = r5l_bio_alloc(log, io);
 	bio_add_page(io->current_bio, io->meta_page, PAGE_SIZE, 0);
 
-	log->seq++;
 	r5_reserve_log_entry(log, io);
 
 	spin_lock_irq(&log->io_list_lock);
-- 
1.9.1


^ permalink raw reply related

* [PATCH 12/12] raid5-cache: use bio chaining
From: Christoph Hellwig @ 2015-09-12  6:17 UTC (permalink / raw)
  To: Shaohua Li, neilb; +Cc: linux-raid, Kernel-team, dan.j.williams
In-Reply-To: <1442038638-6947-1-git-send-email-hch@lst.de>

Simplify the bio completion handler by using bio chaining and submitting
bios as soon as they are full.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 drivers/md/raid5-cache.c | 38 ++++++++++++++++----------------------
 1 file changed, 16 insertions(+), 22 deletions(-)

diff --git a/drivers/md/raid5-cache.c b/drivers/md/raid5-cache.c
index 858bfcf..50ffc9b 100644
--- a/drivers/md/raid5-cache.c
+++ b/drivers/md/raid5-cache.c
@@ -98,8 +98,6 @@ struct r5l_io_unit {
 	struct page *meta_page; /* store meta block */
 	int meta_offset; /* current offset in meta_page */
 
-	struct bio_list bios;
-	atomic_t pending_io; /* pending bios not written to log yet */
 	struct bio *current_bio; /* current_bio accepting new data */
 
 	atomic_t pending_stripe; /* how many stripes not flushed to raid */
@@ -110,6 +108,7 @@ struct r5l_io_unit {
 	struct list_head stripe_list; /* stripes added to the io_unit */
 
 	int state;
+	bool need_split_bio;
 };
 
 /* r5l_io_unit state */
@@ -213,9 +212,6 @@ static void r5l_log_endio(struct bio *bio)
 
 	bio_put(bio);
 
-	if (!atomic_dec_and_test(&io->pending_io))
-		return;
-
 	spin_lock_irqsave(&log->io_list_lock, flags);
 	__r5l_set_io_unit_state(io, IO_UNIT_IO_END);
 	if (log->use_fua)
@@ -233,7 +229,6 @@ static void r5l_submit_current_io(struct r5l_log *log)
 {
 	struct r5l_io_unit *io = log->current_io;
 	struct r5l_meta_block *block;
-	struct bio *bio;
 	unsigned long flags;
 	u32 crc;
 
@@ -250,22 +245,17 @@ static void r5l_submit_current_io(struct r5l_log *log)
 	__r5l_set_io_unit_state(io, IO_UNIT_IO_START);
 	spin_unlock_irqrestore(&log->io_list_lock, flags);
 
-	while ((bio = bio_list_pop(&io->bios)))
-		submit_bio(WRITE | (log->use_fua ? REQ_FUA : 0), bio);
+	submit_bio(WRITE | (log->use_fua ? REQ_FUA : 0), io->current_bio);
 }
 
-static struct bio *r5l_bio_alloc(struct r5l_log *log, struct r5l_io_unit *io)
+static struct bio *r5l_bio_alloc(struct r5l_log *log)
 {
 	struct bio *bio = bio_kmalloc(GFP_NOIO | __GFP_NOFAIL, BIO_MAX_PAGES);
 
 	bio->bi_rw = WRITE;
 	bio->bi_bdev = log->rdev->bdev;
 	bio->bi_iter.bi_sector = log->rdev->data_offset + log->log_start;
-	bio->bi_end_io = r5l_log_endio;
-	bio->bi_private = io;
 
-	bio_list_add(&io->bios, bio);
-	atomic_inc(&io->pending_io);
 	return bio;
 }
 
@@ -281,7 +271,7 @@ static void r5_reserve_log_entry(struct r5l_log *log, struct r5l_io_unit *io)
 	 * of BLOCK_SECTORS.
 	 */
 	if (log->log_start == 0)
-		io->current_bio = NULL;
+		io->need_split_bio = true;
 
 	io->log_end = log->log_start;
 }
@@ -294,7 +284,6 @@ static struct r5l_io_unit *r5l_new_meta(struct r5l_log *log)
 	/* We can't handle memory allocate failure so far */
 	io = kmem_cache_zalloc(log->io_kc, GFP_NOIO | __GFP_NOFAIL);
 	io->log = log;
-	bio_list_init(&io->bios);
 	INIT_LIST_HEAD(&io->log_sibling);
 	INIT_LIST_HEAD(&io->stripe_list);
 	io->state = IO_UNIT_RUNNING;
@@ -310,7 +299,9 @@ static struct r5l_io_unit *r5l_new_meta(struct r5l_log *log)
 	io->meta_offset = sizeof(struct r5l_meta_block);
 	io->seq = log->seq++;
 
-	io->current_bio = r5l_bio_alloc(log, io);
+	io->current_bio = r5l_bio_alloc(log);
+	io->current_bio->bi_end_io = r5l_log_endio;
+	io->current_bio->bi_private = io;
 	bio_add_page(io->current_bio, io->meta_page, PAGE_SIZE, 0);
 
 	r5_reserve_log_entry(log, io);
@@ -358,15 +349,18 @@ static void r5l_append_payload_page(struct r5l_log *log, struct page *page)
 {
 	struct r5l_io_unit *io = log->current_io;
 
-alloc_bio:
-	if (!io->current_bio) 
-		io->current_bio = r5l_bio_alloc(log, io);
+	if (io->need_split_bio) {
+		struct bio *prev = io->current_bio;
 
-	if (!bio_add_page(io->current_bio, page, PAGE_SIZE, 0)) {
-		io->current_bio = NULL;
-		goto alloc_bio;
+		io->current_bio = r5l_bio_alloc(log);
+		bio_chain(io->current_bio, prev);
+
+		submit_bio(WRITE | (log->use_fua ? REQ_FUA : 0), prev);
 	}
 
+	if (!bio_add_page(io->current_bio, page, PAGE_SIZE, 0))
+		BUG();
+
 	r5_reserve_log_entry(log, io);
 }
 
-- 
1.9.1


^ permalink raw reply related

* [PATCH 00/39] drop null test before destroy functions
From: Julia Lawall @ 2015-09-13 12:14 UTC (permalink / raw)
  To: iommu-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA,
	dccp-u79uwXL29TY76Z2rM5mHXA,
	dri-devel-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW,
	intel-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW,
	ecryptfs-u79uwXL29TY76Z2rM5mHXA, dmaengine-u79uwXL29TY76Z2rM5mHXA
  Cc: linux-usb-u79uwXL29TY76Z2rM5mHXA, kvm-u79uwXL29TY76Z2rM5mHXA,
	kernel-janitors-u79uwXL29TY76Z2rM5mHXA,
	HPDD-discuss-hn68Rpc1hR1g9hUCZPvPmw,
	linux-mtd-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
	ocfs2-devel-N0ozoZBvEnrZJqsBc5GL+g, Carolyn Wyborny,
	ceph-devel-u79uwXL29TY76Z2rM5mHXA,
	devel-gWbeCf7V1WCQmaza687I9mD2FQJk+8+b,
	linux-nilfs-u79uwXL29TY76Z2rM5mHXA,
	linux-rdma-u79uwXL29TY76Z2rM5mHXA, Don Skidmore,
	linux-atm-general-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f,
	Jesse Brandeburg, cluster-devel-H+wXaHxf7aLQT0dZR+AlfA,
	Matthew Vick, intel-wired-lan-qjLDD68F18P21nG7glBr7A,
	linux-ext4-u79uwXL29TY76Z2rM5mHXA, Mitch Williams, Chao Yu,
	linux-raid-u79uwXL29TY76Z2rM5mHXA,
	linux-bcache-u79uwXL29TY76Z2rM5mHXA, Dan Williams,
	linux-nfs-u79uwXL29TY76Z2rM5mHXA, netdev-u79uwXL29TY76Z2rM5mHXA,
	Shannon Nelson, linux-kernel-u79uwXL29TaqPxH82wqD4g

Recent commits to kernel/git/torvalds/linux.git have made the following
functions able to tolerate NULL arguments:

kmem_cache_destroy (commit 3942d29918522)
mempool_destroy (commit 4e3ca3e033d1)
dma_pool_destroy (commit 44d7175da6ea)

These patches remove the associated NULL tests for the files that I found
easy to compile test.  If these changes are OK, I will address the
remainder later.

---

 arch/x86/kvm/mmu.c                                 |    6 --
 block/bio-integrity.c                              |    7 --
 block/bio.c                                        |    7 --
 block/blk-core.c                                   |    3 -
 block/elevator.c                                   |    3 -
 drivers/atm/he.c                                   |    7 --
 drivers/block/aoe/aoedev.c                         |    3 -
 drivers/block/drbd/drbd_main.c                     |   21 ++-----
 drivers/block/pktcdvd.c                            |    3 -
 drivers/block/rbd.c                                |    6 --
 drivers/dma/dmaengine.c                            |    6 --
 drivers/firmware/google/gsmi.c                     |    3 -
 drivers/gpu/drm/i915/i915_dma.c                    |   19 ++----
 drivers/iommu/amd_iommu_init.c                     |    7 --
 drivers/md/bcache/bset.c                           |    3 -
 drivers/md/bcache/request.c                        |    3 -
 drivers/md/bcache/super.c                          |    9 +--
 drivers/md/dm-bufio.c                              |    3 -
 drivers/md/dm-cache-target.c                       |    3 -
 drivers/md/dm-crypt.c                              |    6 --
 drivers/md/dm-io.c                                 |    3 -
 drivers/md/dm-log-userspace-base.c                 |    3 -
 drivers/md/dm-region-hash.c                        |    4 -
 drivers/md/dm.c                                    |   13 +---
 drivers/md/multipath.c                             |    3 -
 drivers/md/raid1.c                                 |    6 --
 drivers/md/raid10.c                                |    9 +--
 drivers/md/raid5.c                                 |    3 -
 drivers/mtd/nand/nandsim.c                         |    3 -
 drivers/mtd/ubi/attach.c                           |    4 -
 drivers/net/ethernet/intel/ixgbe/ixgbe_fcoe.c      |    3 -
 drivers/staging/lustre/lustre/llite/super25.c      |   16 +----
 drivers/staging/lustre/lustre/obdclass/genops.c    |   24 ++------
 drivers/staging/lustre/lustre/obdclass/lu_object.c |    6 --
 drivers/staging/rdma/hfi1/user_sdma.c              |    3 -
 drivers/thunderbolt/ctl.c                          |    3 -
 drivers/usb/gadget/udc/bdc/bdc_core.c              |    3 -
 drivers/usb/gadget/udc/gr_udc.c                    |    3 -
 drivers/usb/gadget/udc/mv_u3d_core.c               |    3 -
 drivers/usb/gadget/udc/mv_udc_core.c               |    3 -
 drivers/usb/host/fotg210-hcd.c                     |   12 +---
 drivers/usb/host/fusbh200-hcd.c                    |   12 +---
 drivers/usb/host/whci/init.c                       |    3 -
 drivers/usb/host/xhci-mem.c                        |   12 +---
 fs/btrfs/backref.c                                 |    3 -
 fs/btrfs/delayed-inode.c                           |    3 -
 fs/btrfs/delayed-ref.c                             |   12 +---
 fs/btrfs/disk-io.c                                 |    3 -
 fs/btrfs/extent_io.c                               |    6 --
 fs/btrfs/extent_map.c                              |    3 -
 fs/btrfs/file.c                                    |    3 -
 fs/btrfs/inode.c                                   |   18 ++----
 fs/btrfs/ordered-data.c                            |    3 -
 fs/dlm/memory.c                                    |    6 --
 fs/ecryptfs/main.c                                 |    3 -
 fs/ext4/crypto.c                                   |    9 +--
 fs/ext4/extents_status.c                           |    3 -
 fs/ext4/mballoc.c                                  |    3 -
 fs/f2fs/crypto.c                                   |    9 +--
 fs/gfs2/main.c                                     |   29 ++--------
 fs/jbd2/journal.c                                  |   15 +----
 fs/jbd2/revoke.c                                   |   12 +---
 fs/jbd2/transaction.c                              |    6 --
 fs/jffs2/malloc.c                                  |   27 +++------
 fs/nfsd/nfscache.c                                 |    6 --
 fs/nilfs2/super.c                                  |   12 +---
 fs/ocfs2/dlm/dlmlock.c                             |    3 -
 fs/ocfs2/dlm/dlmmaster.c                           |   16 +----
 fs/ocfs2/super.c                                   |   18 ++----
 fs/ocfs2/uptodate.c                                |    3 -
 lib/debugobjects.c                                 |    3 -
 net/core/sock.c                                    |   12 +---
 net/dccp/ackvec.c                                  |   12 +---
 net/dccp/ccid.c                                    |    3 -
 net/sunrpc/sched.c                                 |   12 +---
 75 files changed, 180 insertions(+), 381 deletions(-)

^ permalink raw reply

* [PATCH 12/39] dm: drop null test before destroy functions
From: Julia Lawall @ 2015-09-13 12:15 UTC (permalink / raw)
  To: Alasdair Kergon
  Cc: sergey.senozhatsky, kernel-janitors, Mike Snitzer, dm-devel,
	Neil Brown, linux-raid, linux-kernel
In-Reply-To: <1442146532-9100-1-git-send-email-Julia.Lawall@lip6.fr>

Remove unneeded NULL test.

The semantic patch that makes this change is as follows:
(http://coccinelle.lip6.fr/)

// <smpl>
@@ expression x; @@
-if (x != NULL)
  \(kmem_cache_destroy\|mempool_destroy\|dma_pool_destroy\)(x);
// </smpl>

Signed-off-by: Julia Lawall <Julia.Lawall@lip6.fr>

---
 drivers/md/dm-bufio.c              |    3 +--
 drivers/md/dm-cache-target.c       |    3 +--
 drivers/md/dm-crypt.c              |    6 ++----
 drivers/md/dm-io.c                 |    3 +--
 drivers/md/dm-log-userspace-base.c |    3 +--
 drivers/md/dm-region-hash.c        |    4 +---
 drivers/md/dm.c                    |   13 ++++---------
 7 files changed, 11 insertions(+), 24 deletions(-)

diff --git a/drivers/md/dm-bufio.c b/drivers/md/dm-bufio.c
index 83cc52e..8ad39b6 100644
--- a/drivers/md/dm-bufio.c
+++ b/drivers/md/dm-bufio.c
@@ -1864,8 +1864,7 @@ static void __exit dm_bufio_exit(void)
 	for (i = 0; i < ARRAY_SIZE(dm_bufio_caches); i++) {
 		struct kmem_cache *kc = dm_bufio_caches[i];
 
-		if (kc)
-			kmem_cache_destroy(kc);
+		kmem_cache_destroy(kc);
 	}
 
 	for (i = 0; i < ARRAY_SIZE(dm_bufio_cache_names); i++)
diff --git a/drivers/md/dm.c b/drivers/md/dm.c
index 6264781..163de31 100644
--- a/drivers/md/dm.c
+++ b/drivers/md/dm.c
@@ -2220,10 +2220,8 @@ static void cleanup_mapped_device(struct mapped_device *md)
 		destroy_workqueue(md->wq);
 	if (md->kworker_task)
 		kthread_stop(md->kworker_task);
-	if (md->io_pool)
-		mempool_destroy(md->io_pool);
-	if (md->rq_pool)
-		mempool_destroy(md->rq_pool);
+	mempool_destroy(md->io_pool);
+	mempool_destroy(md->rq_pool);
 	if (md->bs)
 		bioset_free(md->bs);
 
@@ -3508,11 +3506,8 @@ void dm_free_md_mempools(struct dm_md_mempools *pools)
 	if (!pools)
 		return;
 
-	if (pools->io_pool)
-		mempool_destroy(pools->io_pool);
-
-	if (pools->rq_pool)
-		mempool_destroy(pools->rq_pool);
+	mempool_destroy(pools->io_pool);
+	mempool_destroy(pools->rq_pool);
 
 	if (pools->bs)
 		bioset_free(pools->bs);
diff --git a/drivers/md/dm-log-userspace-base.c b/drivers/md/dm-log-userspace-base.c
index 058256d..53b7b06 100644
--- a/drivers/md/dm-log-userspace-base.c
+++ b/drivers/md/dm-log-userspace-base.c
@@ -313,8 +313,7 @@ static int userspace_ctr(struct dm_dirty_log *log, struct dm_target *ti,
 out:
 	kfree(devices_rdata);
 	if (r) {
-		if (lc->flush_entry_pool)
-			mempool_destroy(lc->flush_entry_pool);
+		mempool_destroy(lc->flush_entry_pool);
 		kfree(lc);
 		kfree(ctr_str);
 	} else {
diff --git a/drivers/md/dm-io.c b/drivers/md/dm-io.c
index 6f8e83b..81c5e1a 100644
--- a/drivers/md/dm-io.c
+++ b/drivers/md/dm-io.c
@@ -65,8 +65,7 @@ struct dm_io_client *dm_io_client_create(void)
 	return client;
 
    bad:
-	if (client->pool)
-		mempool_destroy(client->pool);
+	mempool_destroy(client->pool);
 	kfree(client);
 	return ERR_PTR(-ENOMEM);
 }
diff --git a/drivers/md/dm-cache-target.c b/drivers/md/dm-cache-target.c
index dd90d12..2fd4c82 100644
--- a/drivers/md/dm-cache-target.c
+++ b/drivers/md/dm-cache-target.c
@@ -2309,8 +2309,7 @@ static void destroy(struct cache *cache)
 {
 	unsigned i;
 
-	if (cache->migration_pool)
-		mempool_destroy(cache->migration_pool);
+	mempool_destroy(cache->migration_pool);
 
 	if (cache->all_io_ds)
 		dm_deferred_set_destroy(cache->all_io_ds);
diff --git a/drivers/md/dm-crypt.c b/drivers/md/dm-crypt.c
index d60c88d..cf91a96 100644
--- a/drivers/md/dm-crypt.c
+++ b/drivers/md/dm-crypt.c
@@ -1543,10 +1543,8 @@ static void crypt_dtr(struct dm_target *ti)
 	if (cc->bs)
 		bioset_free(cc->bs);
 
-	if (cc->page_pool)
-		mempool_destroy(cc->page_pool);
-	if (cc->req_pool)
-		mempool_destroy(cc->req_pool);
+	mempool_destroy(cc->page_pool);
+	mempool_destroy(cc->req_pool);
 
 	if (cc->iv_gen_ops && cc->iv_gen_ops->dtr)
 		cc->iv_gen_ops->dtr(cc);
diff --git a/drivers/md/dm-region-hash.c b/drivers/md/dm-region-hash.c
index b929fd5..f3d608b 100644
--- a/drivers/md/dm-region-hash.c
+++ b/drivers/md/dm-region-hash.c
@@ -249,9 +249,7 @@ void dm_region_hash_destroy(struct dm_region_hash *rh)
 	if (rh->log)
 		dm_dirty_log_destroy(rh->log);
 
-	if (rh->region_pool)
-		mempool_destroy(rh->region_pool);
-
+	mempool_destroy(rh->region_pool);
 	vfree(rh->buckets);
 	kfree(rh);
 }

^ permalink raw reply related

* [PATCH 15/39] bcache: drop null test before destroy functions
From: Julia Lawall @ 2015-09-13 12:15 UTC (permalink / raw)
  To: Kent Overstreet
  Cc: sergey.senozhatsky, kernel-janitors, Neil Brown, linux-bcache,
	linux-raid, linux-kernel
In-Reply-To: <1442146532-9100-1-git-send-email-Julia.Lawall@lip6.fr>

Remove unneeded NULL test.

The semantic patch that makes this change is as follows:
(http://coccinelle.lip6.fr/)

// <smpl>
@@ expression x; @@
-if (x != NULL)
  \(kmem_cache_destroy\|mempool_destroy\|dma_pool_destroy\)(x);
// </smpl>

Signed-off-by: Julia Lawall <Julia.Lawall@lip6.fr>

---
 drivers/md/bcache/bset.c    |    3 +--
 drivers/md/bcache/request.c |    3 +--
 drivers/md/bcache/super.c   |    9 +++------
 3 files changed, 5 insertions(+), 10 deletions(-)

diff --git a/drivers/md/bcache/request.c b/drivers/md/bcache/request.c
index 8e9877b..bd4678c 100644
--- a/drivers/md/bcache/request.c
+++ b/drivers/md/bcache/request.c
@@ -1145,8 +1145,7 @@ void bch_flash_dev_request_init(struct bcache_device *d)
 
 void bch_request_exit(void)
 {
-	if (bch_search_cache)
-		kmem_cache_destroy(bch_search_cache);
+	kmem_cache_destroy(bch_search_cache);
 }
 
 int __init bch_request_init(void)
diff --git a/drivers/md/bcache/bset.c b/drivers/md/bcache/bset.c
index 646fe85..479117c 100644
--- a/drivers/md/bcache/bset.c
+++ b/drivers/md/bcache/bset.c
@@ -1116,8 +1116,7 @@ struct bkey *bch_btree_iter_next_filter(struct btree_iter *iter,
 
 void bch_bset_sort_state_free(struct bset_sort_state *state)
 {
-	if (state->pool)
-		mempool_destroy(state->pool);
+	mempool_destroy(state->pool);
 }
 
 int bch_bset_sort_state_init(struct bset_sort_state *state, unsigned page_order)
diff --git a/drivers/md/bcache/super.c b/drivers/md/bcache/super.c
index 679a093..c27a0ff 100644
--- a/drivers/md/bcache/super.c
+++ b/drivers/md/bcache/super.c
@@ -1335,12 +1335,9 @@ static void cache_set_free(struct closure *cl)
 		destroy_workqueue(c->moving_gc_wq);
 	if (c->bio_split)
 		bioset_free(c->bio_split);
-	if (c->fill_iter)
-		mempool_destroy(c->fill_iter);
-	if (c->bio_meta)
-		mempool_destroy(c->bio_meta);
-	if (c->search)
-		mempool_destroy(c->search);
+	mempool_destroy(c->fill_iter);
+	mempool_destroy(c->bio_meta);
+	mempool_destroy(c->search);
 	kfree(c->devices);
 
 	mutex_lock(&bch_register_lock);

^ permalink raw reply related

* [PATCH 17/39] md: drop null test before destroy functions
From: Julia Lawall @ 2015-09-13 12:15 UTC (permalink / raw)
  To: Neil Brown; +Cc: sergey.senozhatsky, kernel-janitors, linux-raid, linux-kernel
In-Reply-To: <1442146532-9100-1-git-send-email-Julia.Lawall@lip6.fr>

Remove unneeded NULL test.

The semantic patch that makes this change is as follows:
(http://coccinelle.lip6.fr/)

// <smpl>
@@ expression x; @@
-if (x != NULL)
  \(kmem_cache_destroy\|mempool_destroy\|dma_pool_destroy\)(x);
// </smpl>

Signed-off-by: Julia Lawall <Julia.Lawall@lip6.fr>

---
 drivers/md/multipath.c |    3 +--
 drivers/md/raid1.c     |    6 ++----
 drivers/md/raid10.c    |    9 +++------
 drivers/md/raid5.c     |    3 +--
 4 files changed, 7 insertions(+), 14 deletions(-)

diff --git a/drivers/md/raid5.c b/drivers/md/raid5.c
index 15ef2c6..09a12d7 100644
--- a/drivers/md/raid5.c
+++ b/drivers/md/raid5.c
@@ -2271,8 +2271,7 @@ static void shrink_stripes(struct r5conf *conf)
 	       drop_one_stripe(conf))
 		;
 
-	if (conf->slab_cache)
-		kmem_cache_destroy(conf->slab_cache);
+	kmem_cache_destroy(conf->slab_cache);
 	conf->slab_cache = NULL;
 }
 
diff --git a/drivers/md/raid1.c b/drivers/md/raid1.c
index 4517f06..5f4f553 100644
--- a/drivers/md/raid1.c
+++ b/drivers/md/raid1.c
@@ -2843,8 +2843,7 @@ static struct r1conf *setup_conf(struct mddev *mddev)
 
  abort:
 	if (conf) {
-		if (conf->r1bio_pool)
-			mempool_destroy(conf->r1bio_pool);
+		mempool_destroy(conf->r1bio_pool);
 		kfree(conf->mirrors);
 		safe_put_page(conf->tmppage);
 		kfree(conf->poolinfo);
@@ -2946,8 +2945,7 @@ static void raid1_free(struct mddev *mddev, void *priv)
 {
 	struct r1conf *conf = priv;
 
-	if (conf->r1bio_pool)
-		mempool_destroy(conf->r1bio_pool);
+	mempool_destroy(conf->r1bio_pool);
 	kfree(conf->mirrors);
 	safe_put_page(conf->tmppage);
 	kfree(conf->poolinfo);
diff --git a/drivers/md/multipath.c b/drivers/md/multipath.c
index d222522..d132f06 100644
--- a/drivers/md/multipath.c
+++ b/drivers/md/multipath.c
@@ -470,8 +470,7 @@ static int multipath_run (struct mddev *mddev)
 	return 0;
 
 out_free_conf:
-	if (conf->pool)
-		mempool_destroy(conf->pool);
+	mempool_destroy(conf->pool);
 	kfree(conf->multipaths);
 	kfree(conf);
 	mddev->private = NULL;
diff --git a/drivers/md/raid10.c b/drivers/md/raid10.c
index 0fc33eb..7c99a40 100644
--- a/drivers/md/raid10.c
+++ b/drivers/md/raid10.c
@@ -3486,8 +3486,7 @@ static struct r10conf *setup_conf(struct mddev *mddev)
 		printk(KERN_ERR "md/raid10:%s: couldn't allocate memory.\n",
 		       mdname(mddev));
 	if (conf) {
-		if (conf->r10bio_pool)
-			mempool_destroy(conf->r10bio_pool);
+		mempool_destroy(conf->r10bio_pool);
 		kfree(conf->mirrors);
 		safe_put_page(conf->tmppage);
 		kfree(conf);
@@ -3682,8 +3681,7 @@ static int run(struct mddev *mddev)
 
 out_free_conf:
 	md_unregister_thread(&mddev->thread);
-	if (conf->r10bio_pool)
-		mempool_destroy(conf->r10bio_pool);
+	mempool_destroy(conf->r10bio_pool);
 	safe_put_page(conf->tmppage);
 	kfree(conf->mirrors);
 	kfree(conf);
@@ -3696,8 +3694,7 @@ static void raid10_free(struct mddev *mddev, void *priv)
 {
 	struct r10conf *conf = priv;
 
-	if (conf->r10bio_pool)
-		mempool_destroy(conf->r10bio_pool);
+	mempool_destroy(conf->r10bio_pool);
 	safe_put_page(conf->tmppage);
 	kfree(conf->mirrors);
 	kfree(conf->mirrors_old);

^ permalink raw reply related

* mdstat reporting (superblock & status side-by-side). Can it be changed ?
From: David C. Rankin @ 2015-09-13 17:58 UTC (permalink / raw)
  To: mdraid

All,

   After looking at the output of /proc/mdstat many times, there is one 'human 
factors' (human nature) bit of difficulty with the order of the output 
(especially with a standard 2-disk raid1 array and 1.2 superblock:

md1 : active raid1 sda7[2] sdb7[1]
       52396032 blocks super 1.2 [2/2] [UU]
                             ^^^^^^^^^

   When scanning the information quickly to insure you do NOT have 1/2 disks 
active in the array, you run into the version and disk information side-by-side.

   "1.2 [2/2]"

   Which naturally causes a short "huh?..., OK" double-take.

   Is there any way from a user standpoint (short of "read a b c d e f"; echo 
"$c $d $a $b $e $f"), to rearrange the order the information is reported as:

md1 : active raid1 sda7[2] sdb7[1]
       super 1.2 52396032 blocks [2/2] [UU]

   That simple change makes glancing down the right-side of the information much 
easier on the brain....

   I don't think this was ever a concern prior to the 1.2 superbock and I'm not 
sure it warrants any brainpower now. However, it was something that stood out to 
me, so I thought I would pass it along.

-- 
David C. Rankin, J.D.,P.E.

^ permalink raw reply

* Re: mdstat reporting (superblock & status side-by-side). Can it be changed ?
From: Roman Mamedov @ 2015-09-13 18:09 UTC (permalink / raw)
  To: David C. Rankin; +Cc: mdraid
In-Reply-To: <55F5B941.8050107@suddenlinkmail.com>

[-- Attachment #1: Type: text/plain, Size: 1916 bytes --]

On Sun, 13 Sep 2015 12:58:25 -0500
"David C. Rankin" <drankinatty@suddenlinkmail.com> wrote:

>    After looking at the output of /proc/mdstat many times, there is one 'human 
> factors' (human nature) bit of difficulty with the order of the output 
> (especially with a standard 2-disk raid1 array and 1.2 superblock:
> 
> md1 : active raid1 sda7[2] sdb7[1]
>        52396032 blocks super 1.2 [2/2] [UU]
>                              ^^^^^^^^^
> 
>    When scanning the information quickly to insure you do NOT have 1/2 disks 
> active in the array, you run into the version and disk information side-by-side.
> 
>    "1.2 [2/2]"
> 
>    Which naturally causes a short "huh?..., OK" double-take.

For me it does not. 1.2 is clearly different from 1/2, and the actual 2/2
that you have is even enclosed in brackets to stand out more prominently.

>    Is there any way from a user standpoint (short of "read a b c d e f"; echo 
> "$c $d $a $b $e $f"), to rearrange the order the information is reported as:
> 
> md1 : active raid1 sda7[2] sdb7[1]
>        super 1.2 52396032 blocks [2/2] [UU]
> 
>    That simple change makes glancing down the right-side of the information much 
> easier on the brain....

If you want to simplify the mental processing, just look for any "_"s in the
string of "U"s which is also conveniently placed at the end of the line. In a
partially down RAID1 you'd have [U_] or [_U] there, which is very noticeable
compared to the usual [UU].

>    I don't think this was ever a concern prior to the 1.2 superbock and I'm not 
> sure it warrants any brainpower now. However, it was something that stood out to 
> me, so I thought I would pass it along.

I don't think it's justified to randomly change things around which many shell
scripts and the like are quite likely to depend on, just out of a personal
preference.

-- 
With respect,
Roman

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

^ permalink raw reply

* Re: [PATCH 15/39] bcache: drop null test before destroy functions
From: Johannes Thumshirn @ 2015-09-14  9:08 UTC (permalink / raw)
  To: Julia Lawall
  Cc: Kent Overstreet, sergey.senozhatsky, kernel-janitors, Neil Brown,
	linux-bcache, linux-raid, linux-kernel
In-Reply-To: <1442146532-9100-16-git-send-email-Julia.Lawall@lip6.fr>

Julia Lawall <Julia.Lawall@lip6.fr> writes:

> Remove unneeded NULL test.
>
> The semantic patch that makes this change is as follows:
> (http://coccinelle.lip6.fr/)
>
> // <smpl>
> @@ expression x; @@
> -if (x != NULL)
>   \(kmem_cache_destroy\|mempool_destroy\|dma_pool_destroy\)(x);
> // </smpl>
>
> Signed-off-by: Julia Lawall <Julia.Lawall@lip6.fr>

Reviewed-by: Johannes Thumshirn <jthumshirn@suse.de>

>
> ---
>  drivers/md/bcache/bset.c    |    3 +--
>  drivers/md/bcache/request.c |    3 +--
>  drivers/md/bcache/super.c   |    9 +++------
>  3 files changed, 5 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/md/bcache/request.c b/drivers/md/bcache/request.c
> index 8e9877b..bd4678c 100644
> --- a/drivers/md/bcache/request.c
> +++ b/drivers/md/bcache/request.c
> @@ -1145,8 +1145,7 @@ void bch_flash_dev_request_init(struct bcache_device *d)
>  
>  void bch_request_exit(void)
>  {
> -	if (bch_search_cache)
> -		kmem_cache_destroy(bch_search_cache);
> +	kmem_cache_destroy(bch_search_cache);
>  }
>  
>  int __init bch_request_init(void)
> diff --git a/drivers/md/bcache/bset.c b/drivers/md/bcache/bset.c
> index 646fe85..479117c 100644
> --- a/drivers/md/bcache/bset.c
> +++ b/drivers/md/bcache/bset.c
> @@ -1116,8 +1116,7 @@ struct bkey *bch_btree_iter_next_filter(struct btree_iter *iter,
>  
>  void bch_bset_sort_state_free(struct bset_sort_state *state)
>  {
> -	if (state->pool)
> -		mempool_destroy(state->pool);
> +	mempool_destroy(state->pool);
>  }
>  
>  int bch_bset_sort_state_init(struct bset_sort_state *state, unsigned page_order)
> diff --git a/drivers/md/bcache/super.c b/drivers/md/bcache/super.c
> index 679a093..c27a0ff 100644
> --- a/drivers/md/bcache/super.c
> +++ b/drivers/md/bcache/super.c
> @@ -1335,12 +1335,9 @@ static void cache_set_free(struct closure *cl)
>  		destroy_workqueue(c->moving_gc_wq);
>  	if (c->bio_split)
>  		bioset_free(c->bio_split);
> -	if (c->fill_iter)
> -		mempool_destroy(c->fill_iter);
> -	if (c->bio_meta)
> -		mempool_destroy(c->bio_meta);
> -	if (c->search)
> -		mempool_destroy(c->search);
> +	mempool_destroy(c->fill_iter);
> +	mempool_destroy(c->bio_meta);
> +	mempool_destroy(c->search);
>  	kfree(c->devices);
>  
>  	mutex_lock(&bch_register_lock);
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-bcache" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

-- 
Johannes Thumshirn                                           Storage
jthumshirn@suse.de                                 +49 911 74053 689
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: Felix Imendörffer, Jane Smithard, Graham Norton
HRB 21284 (AG Nürnberg)
Key fingerprint = EC38 9CAB C2C4 F25D 8600  D0D0 0393 969D 2D76 0850

^ permalink raw reply

* Re: [PATCH 00/39] drop null test before destroy functions
From: SF Markus Elfring @ 2015-09-14 11:55 UTC (permalink / raw)
  To: Julia Lawall, kernel-janitors
  Cc: iommu, dccp, dri-devel, intel-gfx, ecryptfs, dmaengine,
	sergey.senozhatsky, Dan Williams, cluster-devel, intel-wired-lan,
	Mitch Williams, John Ronciak, Matthew Vick, Don Skidmore,
	Carolyn Wyborny, Shannon Nelson, Jesse Brandeburg, linux-nfs,
	linux-raid, netdev, linux-atm-general, linux-btrfs, linux-nilfs,
	linux-kernel, linux-mtd, linux-usb
In-Reply-To: <1442146532-9100-1-git-send-email-Julia.Lawall@lip6.fr>

> Recent commits to kernel/git/torvalds/linux.git have made the following
> functions able to tolerate NULL arguments:
>
> kmem_cache_destroy (commit 3942d29918522)
> mempool_destroy (commit 4e3ca3e033d1)
> dma_pool_destroy (commit 44d7175da6ea)

How do you think about to extend an other SmPL script?

Related topic:
scripts/coccinelle/free: Delete NULL test before freeing functions
https://systeme.lip6.fr/pipermail/cocci/2015-May/001960.html
https://www.mail-archive.com/cocci@systeme.lip6.fr/msg01855.html


> If these changes are OK, I will address the remainder later.

Would anybody like to reuse my general SmPL approach for similar source
code clean-up?

Regards,
Markus

^ permalink raw reply

* Re: [dm-devel] [PATCH 12/39] dm: drop null test before destroy functions
From: Mikulas Patocka @ 2015-09-14 13:46 UTC (permalink / raw)
  To: device-mapper development
  Cc: Alasdair Kergon, Mike Snitzer, kernel-janitors, Neil Brown,
	linux-kernel, linux-raid, sergey.senozhatsky
In-Reply-To: <1442146532-9100-13-git-send-email-Julia.Lawall@lip6.fr>



On Sun, 13 Sep 2015, Julia Lawall wrote:

> Remove unneeded NULL test.
> 
> The semantic patch that makes this change is as follows:
> (http://coccinelle.lip6.fr/)
> 
> // <smpl>
> @@ expression x; @@
> -if (x != NULL)
>   \(kmem_cache_destroy\|mempool_destroy\|dma_pool_destroy\)(x);
> // </smpl>
> 
> Signed-off-by: Julia Lawall <Julia.Lawall@lip6.fr>
> 
> ---
>  drivers/md/dm-bufio.c              |    3 +--
>  drivers/md/dm-cache-target.c       |    3 +--
>  drivers/md/dm-crypt.c              |    6 ++----
>  drivers/md/dm-io.c                 |    3 +--
>  drivers/md/dm-log-userspace-base.c |    3 +--
>  drivers/md/dm-region-hash.c        |    4 +---
>  drivers/md/dm.c                    |   13 ++++---------
>  7 files changed, 11 insertions(+), 24 deletions(-)
> 
> diff --git a/drivers/md/dm-bufio.c b/drivers/md/dm-bufio.c
> index 83cc52e..8ad39b6 100644
> --- a/drivers/md/dm-bufio.c
> +++ b/drivers/md/dm-bufio.c
> @@ -1864,8 +1864,7 @@ static void __exit dm_bufio_exit(void)
>  	for (i = 0; i < ARRAY_SIZE(dm_bufio_caches); i++) {
>  		struct kmem_cache *kc = dm_bufio_caches[i];
>  
> -		if (kc)
> -			kmem_cache_destroy(kc);
> +		kmem_cache_destroy(kc);
>  	}

The variable here can be NULL. I don't know how did you conclude that it 
cannot. It seems that you didn't test the patch, if you did, you'd hit 
NULL pointer dereference here.

>  	for (i = 0; i < ARRAY_SIZE(dm_bufio_cache_names); i++)
> diff --git a/drivers/md/dm.c b/drivers/md/dm.c
> index 6264781..163de31 100644
> --- a/drivers/md/dm.c
> +++ b/drivers/md/dm.c
> @@ -2220,10 +2220,8 @@ static void cleanup_mapped_device(struct mapped_device *md)
>  		destroy_workqueue(md->wq);
>  	if (md->kworker_task)
>  		kthread_stop(md->kworker_task);
> -	if (md->io_pool)
> -		mempool_destroy(md->io_pool);
> -	if (md->rq_pool)
> -		mempool_destroy(md->rq_pool);
> +	mempool_destroy(md->io_pool);
> +	mempool_destroy(md->rq_pool);

Likewise, these variables can be NULL.

>  	if (md->bs)
>  		bioset_free(md->bs);
>  
> @@ -3508,11 +3506,8 @@ void dm_free_md_mempools(struct dm_md_mempools *pools)
>  	if (!pools)
>  		return;
>  
> -	if (pools->io_pool)
> -		mempool_destroy(pools->io_pool);
> -
> -	if (pools->rq_pool)
> -		mempool_destroy(pools->rq_pool);
> +	mempool_destroy(pools->io_pool);
> +	mempool_destroy(pools->rq_pool);
>  
>  	if (pools->bs)
>  		bioset_free(pools->bs);

Likewise, it can be NULL, see for example this code path:
                pools->io_pool = mempool_create_slab_pool(pool_size, cachep);
                if (!pools->io_pool)
                        goto out;
out:
        dm_free_md_mempools(pools);
dm_free_md_mempools:
        if (pools->io_pool)
                mempool_destroy(pools->io_pool);

It seems that you set up the cocinelle tool incorrectly, so that it 
produces many bogus suggestions.

Mikulas

> diff --git a/drivers/md/dm-log-userspace-base.c b/drivers/md/dm-log-userspace-base.c
> index 058256d..53b7b06 100644
> --- a/drivers/md/dm-log-userspace-base.c
> +++ b/drivers/md/dm-log-userspace-base.c
> @@ -313,8 +313,7 @@ static int userspace_ctr(struct dm_dirty_log *log, struct dm_target *ti,
>  out:
>  	kfree(devices_rdata);
>  	if (r) {
> -		if (lc->flush_entry_pool)
> -			mempool_destroy(lc->flush_entry_pool);
> +		mempool_destroy(lc->flush_entry_pool);
>  		kfree(lc);
>  		kfree(ctr_str);
>  	} else {
> diff --git a/drivers/md/dm-io.c b/drivers/md/dm-io.c
> index 6f8e83b..81c5e1a 100644
> --- a/drivers/md/dm-io.c
> +++ b/drivers/md/dm-io.c
> @@ -65,8 +65,7 @@ struct dm_io_client *dm_io_client_create(void)
>  	return client;
>  
>     bad:
> -	if (client->pool)
> -		mempool_destroy(client->pool);
> +	mempool_destroy(client->pool);
>  	kfree(client);
>  	return ERR_PTR(-ENOMEM);
>  }
> diff --git a/drivers/md/dm-cache-target.c b/drivers/md/dm-cache-target.c
> index dd90d12..2fd4c82 100644
> --- a/drivers/md/dm-cache-target.c
> +++ b/drivers/md/dm-cache-target.c
> @@ -2309,8 +2309,7 @@ static void destroy(struct cache *cache)
>  {
>  	unsigned i;
>  
> -	if (cache->migration_pool)
> -		mempool_destroy(cache->migration_pool);
> +	mempool_destroy(cache->migration_pool);
>  
>  	if (cache->all_io_ds)
>  		dm_deferred_set_destroy(cache->all_io_ds);
> diff --git a/drivers/md/dm-crypt.c b/drivers/md/dm-crypt.c
> index d60c88d..cf91a96 100644
> --- a/drivers/md/dm-crypt.c
> +++ b/drivers/md/dm-crypt.c
> @@ -1543,10 +1543,8 @@ static void crypt_dtr(struct dm_target *ti)
>  	if (cc->bs)
>  		bioset_free(cc->bs);
>  
> -	if (cc->page_pool)
> -		mempool_destroy(cc->page_pool);
> -	if (cc->req_pool)
> -		mempool_destroy(cc->req_pool);
> +	mempool_destroy(cc->page_pool);
> +	mempool_destroy(cc->req_pool);
>  
>  	if (cc->iv_gen_ops && cc->iv_gen_ops->dtr)
>  		cc->iv_gen_ops->dtr(cc);
> diff --git a/drivers/md/dm-region-hash.c b/drivers/md/dm-region-hash.c
> index b929fd5..f3d608b 100644
> --- a/drivers/md/dm-region-hash.c
> +++ b/drivers/md/dm-region-hash.c
> @@ -249,9 +249,7 @@ void dm_region_hash_destroy(struct dm_region_hash *rh)
>  	if (rh->log)
>  		dm_dirty_log_destroy(rh->log);
>  
> -	if (rh->region_pool)
> -		mempool_destroy(rh->region_pool);
> -
> +	mempool_destroy(rh->region_pool);
>  	vfree(rh->buckets);
>  	kfree(rh);
>  }
> 
> --
> dm-devel mailing list
> dm-devel@redhat.com
> https://www.redhat.com/mailman/listinfo/dm-devel
> 

^ permalink raw reply

* Re: [PATCH 12/39] dm: drop null test before destroy functions
From: Mike Snitzer @ 2015-09-14 14:00 UTC (permalink / raw)
  To: Mikulas Patocka
  Cc: device-mapper development, Alasdair Kergon, kernel-janitors,
	Neil Brown, linux-kernel, linux-raid, sergey.senozhatsky
In-Reply-To: <alpine.LRH.2.02.1509140932400.7970@file01.intranet.prod.int.rdu2.redhat.com>

On Mon, Sep 14 2015 at  9:46am -0400,
Mikulas Patocka <mpatocka@redhat.com> wrote:

> 
> 
> On Sun, 13 Sep 2015, Julia Lawall wrote:
> 
> > Remove unneeded NULL test.
> > 
> > The semantic patch that makes this change is as follows:
> > (http://coccinelle.lip6.fr/)
> > 
> > // <smpl>
> > @@ expression x; @@
> > -if (x != NULL)
> >   \(kmem_cache_destroy\|mempool_destroy\|dma_pool_destroy\)(x);
> > // </smpl>
> > 
> > Signed-off-by: Julia Lawall <Julia.Lawall@lip6.fr>
> > 
> > ---
> >  drivers/md/dm-bufio.c              |    3 +--
> >  drivers/md/dm-cache-target.c       |    3 +--
> >  drivers/md/dm-crypt.c              |    6 ++----
> >  drivers/md/dm-io.c                 |    3 +--
> >  drivers/md/dm-log-userspace-base.c |    3 +--
> >  drivers/md/dm-region-hash.c        |    4 +---
> >  drivers/md/dm.c                    |   13 ++++---------
> >  7 files changed, 11 insertions(+), 24 deletions(-)
> > 
> > diff --git a/drivers/md/dm-bufio.c b/drivers/md/dm-bufio.c
> > index 83cc52e..8ad39b6 100644
> > --- a/drivers/md/dm-bufio.c
> > +++ b/drivers/md/dm-bufio.c
> > @@ -1864,8 +1864,7 @@ static void __exit dm_bufio_exit(void)
> >  	for (i = 0; i < ARRAY_SIZE(dm_bufio_caches); i++) {
> >  		struct kmem_cache *kc = dm_bufio_caches[i];
> >  
> > -		if (kc)
> > -			kmem_cache_destroy(kc);
> > +		kmem_cache_destroy(kc);
> >  	}
> 
> The variable here can be NULL. I don't know how did you conclude that it 
> cannot. It seems that you didn't test the patch, if you did, you'd hit 
> NULL pointer dereference here.

kmem_cache_destroy(), mempool_destroy(), etc all check for NULL and just
return.  So there is no need for the callers to check for NULL too.

Mike

^ permalink raw reply

* Re: [PATCH 12/39] dm: drop null test before destroy functions
From: Mikulas Patocka @ 2015-09-14 14:05 UTC (permalink / raw)
  To: Mike Snitzer
  Cc: device-mapper development, Alasdair Kergon, kernel-janitors,
	Neil Brown, linux-kernel, linux-raid, sergey.senozhatsky
In-Reply-To: <20150914140040.GA11815@redhat.com>



On Mon, 14 Sep 2015, Mike Snitzer wrote:

> On Mon, Sep 14 2015 at  9:46am -0400,
> Mikulas Patocka <mpatocka@redhat.com> wrote:
> 
> > 
> > 
> > On Sun, 13 Sep 2015, Julia Lawall wrote:
> > 
> > > Remove unneeded NULL test.
> > > 
> > > The semantic patch that makes this change is as follows:
> > > (http://coccinelle.lip6.fr/)
> > > 
> > > // <smpl>
> > > @@ expression x; @@
> > > -if (x != NULL)
> > >   \(kmem_cache_destroy\|mempool_destroy\|dma_pool_destroy\)(x);
> > > // </smpl>
> > > 
> > > Signed-off-by: Julia Lawall <Julia.Lawall@lip6.fr>
> > > 
> > > ---
> > >  drivers/md/dm-bufio.c              |    3 +--
> > >  drivers/md/dm-cache-target.c       |    3 +--
> > >  drivers/md/dm-crypt.c              |    6 ++----
> > >  drivers/md/dm-io.c                 |    3 +--
> > >  drivers/md/dm-log-userspace-base.c |    3 +--
> > >  drivers/md/dm-region-hash.c        |    4 +---
> > >  drivers/md/dm.c                    |   13 ++++---------
> > >  7 files changed, 11 insertions(+), 24 deletions(-)
> > > 
> > > diff --git a/drivers/md/dm-bufio.c b/drivers/md/dm-bufio.c
> > > index 83cc52e..8ad39b6 100644
> > > --- a/drivers/md/dm-bufio.c
> > > +++ b/drivers/md/dm-bufio.c
> > > @@ -1864,8 +1864,7 @@ static void __exit dm_bufio_exit(void)
> > >  	for (i = 0; i < ARRAY_SIZE(dm_bufio_caches); i++) {
> > >  		struct kmem_cache *kc = dm_bufio_caches[i];
> > >  
> > > -		if (kc)
> > > -			kmem_cache_destroy(kc);
> > > +		kmem_cache_destroy(kc);
> > >  	}
> > 
> > The variable here can be NULL. I don't know how did you conclude that it 
> > cannot. It seems that you didn't test the patch, if you did, you'd hit 
> > NULL pointer dereference here.
> 
> kmem_cache_destroy(), mempool_destroy(), etc all check for NULL and just
> return.  So there is no need for the callers to check for NULL too.
> 
> Mike

I see. It was recent change that I missed.

Mikulas

^ permalink raw reply

* Re: [dm-devel] [PATCH 12/39] dm: drop null test before destroy functions
From: walter harms @ 2015-09-14 14:23 UTC (permalink / raw)
  To: Mikulas Patocka
  Cc: device-mapper development, Alasdair Kergon, Mike Snitzer,
	kernel-janitors, Neil Brown, linux-kernel, linux-raid,
	sergey.senozhatsky
In-Reply-To: <alpine.LRH.2.02.1509140932400.7970@file01.intranet.prod.int.rdu2.redhat.com>



Am 14.09.2015 15:46, schrieb Mikulas Patocka:
> 
> On Sun, 13 Sep 2015, Julia Lawall wrote:
> 
>> > Remove unneeded NULL test.
>> > 
>> > The semantic patch that makes this change is as follows:
>> > (http://coccinelle.lip6.fr/)
>> > 
>> > // <smpl>
>> > @@ expression x; @@
>> > -if (x != NULL)
>> >   \(kmem_cache_destroy\|mempool_destroy\|dma_pool_destroy\)(x);
>> > // </smpl>
>> > 
>> > Signed-off-by: Julia Lawall <Julia.Lawall@lip6.fr>
>> > 
>> > ---
>> >  drivers/md/dm-bufio.c              |    3 +--
>> >  drivers/md/dm-cache-target.c       |    3 +--
>> >  drivers/md/dm-crypt.c              |    6 ++----
>> >  drivers/md/dm-io.c                 |    3 +--
>> >  drivers/md/dm-log-userspace-base.c |    3 +--
>> >  drivers/md/dm-region-hash.c        |    4 +---
>> >  drivers/md/dm.c                    |   13 ++++---------
>> >  7 files changed, 11 insertions(+), 24 deletions(-)
>> > 
>> > diff --git a/drivers/md/dm-bufio.c b/drivers/md/dm-bufio.c
>> > index 83cc52e..8ad39b6 100644
>> > --- a/drivers/md/dm-bufio.c
>> > +++ b/drivers/md/dm-bufio.c
>> > @@ -1864,8 +1864,7 @@ static void __exit dm_bufio_exit(void)
>> >  	for (i = 0; i < ARRAY_SIZE(dm_bufio_caches); i++) {
>> >  		struct kmem_cache *kc = dm_bufio_caches[i];
>> >  
>> > -		if (kc)
>> > -			kmem_cache_destroy(kc);
>> > +		kmem_cache_destroy(kc);
>> >  	}


maybe you want to remove "kc" also
by calling kmem_cache_destroy(dm_bufio_caches[i]); directly ?

just a hint,
 wh

^ permalink raw reply

* Apply for loan at 3.9% interest rate
From: Loan @ 2015-09-14 14:47 UTC (permalink / raw)
  To: Loan

Greetings to You

Are you a business man or woman? Are you in any financial mess or do you
need loan to start up your own business? Do you need loan to
settle your debt, pay off your bills or start a nice business?

Do you have a low credit score and you are finding it hard to obtain
capital loan from local banks/other financial institutes 

We are offer all types of loan at a low interest rate of 3.9% percent
without any collateral. (Only identification proof)

If you are interested please contact us today for
the loan application form and more details

Regards

^ permalink raw reply

* Re: [PATCH 12/39] dm: drop null test before destroy functions
From: Julia Lawall @ 2015-09-14 18:50 UTC (permalink / raw)
  To: Mikulas Patocka
  Cc: Mike Snitzer, device-mapper development, Alasdair Kergon,
	kernel-janitors, Neil Brown, linux-kernel, linux-raid,
	sergey.senozhatsky
In-Reply-To: <alpine.LRH.2.02.1509141004430.7970@file01.intranet.prod.int.rdu2.redhat.com>



On Mon, 14 Sep 2015, Mikulas Patocka wrote:

> 
> 
> On Mon, 14 Sep 2015, Mike Snitzer wrote:
> 
> > On Mon, Sep 14 2015 at  9:46am -0400,
> > Mikulas Patocka <mpatocka@redhat.com> wrote:
> > 
> > > 
> > > 
> > > On Sun, 13 Sep 2015, Julia Lawall wrote:
> > > 
> > > > Remove unneeded NULL test.
> > > > 
> > > > The semantic patch that makes this change is as follows:
> > > > (http://coccinelle.lip6.fr/)
> > > > 
> > > > // <smpl>
> > > > @@ expression x; @@
> > > > -if (x != NULL)
> > > >   \(kmem_cache_destroy\|mempool_destroy\|dma_pool_destroy\)(x);
> > > > // </smpl>
> > > > 
> > > > Signed-off-by: Julia Lawall <Julia.Lawall@lip6.fr>
> > > > 
> > > > ---
> > > >  drivers/md/dm-bufio.c              |    3 +--
> > > >  drivers/md/dm-cache-target.c       |    3 +--
> > > >  drivers/md/dm-crypt.c              |    6 ++----
> > > >  drivers/md/dm-io.c                 |    3 +--
> > > >  drivers/md/dm-log-userspace-base.c |    3 +--
> > > >  drivers/md/dm-region-hash.c        |    4 +---
> > > >  drivers/md/dm.c                    |   13 ++++---------
> > > >  7 files changed, 11 insertions(+), 24 deletions(-)
> > > > 
> > > > diff --git a/drivers/md/dm-bufio.c b/drivers/md/dm-bufio.c
> > > > index 83cc52e..8ad39b6 100644
> > > > --- a/drivers/md/dm-bufio.c
> > > > +++ b/drivers/md/dm-bufio.c
> > > > @@ -1864,8 +1864,7 @@ static void __exit dm_bufio_exit(void)
> > > >  	for (i = 0; i < ARRAY_SIZE(dm_bufio_caches); i++) {
> > > >  		struct kmem_cache *kc = dm_bufio_caches[i];
> > > >  
> > > > -		if (kc)
> > > > -			kmem_cache_destroy(kc);
> > > > +		kmem_cache_destroy(kc);
> > > >  	}
> > > 
> > > The variable here can be NULL. I don't know how did you conclude that it 
> > > cannot. It seems that you didn't test the patch, if you did, you'd hit 
> > > NULL pointer dereference here.
> > 
> > kmem_cache_destroy(), mempool_destroy(), etc all check for NULL and just
> > return.  So there is no need for the callers to check for NULL too.
> > 
> > Mike
> 
> I see. It was recent change that I missed.

The relevant commits were in the cover letter:

kmem_cache_destroy (commit 3942d29918522)
mempool_destroy (commit 4e3ca3e033d1)
dma_pool_destroy (commit 44d7175da6ea)

julia

^ permalink raw reply


This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox