Patchwork [RFC] dm: Fix a bio leak in dec_pending()

login
register
mail settings
Submitter Bart Van Assche
Date Aug. 25, 2016, 8:57 p.m.
Message ID <1e64fe53-ed57-ef97-a077-19a153506e63@sandisk.com>
Download mbox | patch
Permalink /patch/9299957/
State New
Headers show

Comments

Bart Van Assche - Aug. 25, 2016, 8:57 p.m.
Ensure that bio_endio() is called if io->error == DM_ENDIO_REQUEUE and
__noflush_suspending(md) returns false. Posting this as an RFC since I'm
not really familiar with the dm code.

Fixes: commit 2e93ccc1933d ("dm: suspend: add noflush pushback")
Signed-off-by: Bart Van Assche <bart.vanassche@sandisk.com>
Cc: Kiyoshi Ueda <k-ueda@ct.jp.nec.com>
---
 drivers/md/dm.c | 16 +++++++++-------
 1 file changed, 9 insertions(+), 7 deletions(-)
Mike Snitzer - Aug. 26, 2016, 2:57 p.m.
On Thu, Aug 25 2016 at  4:57pm -0400,
Bart Van Assche <bart.vanassche@sandisk.com> wrote:

> Ensure that bio_endio() is called if io->error == DM_ENDIO_REQUEUE and
> __noflush_suspending(md) returns false. Posting this as an RFC since I'm
> not really familiar with the dm code.
> 
> Fixes: commit 2e93ccc1933d ("dm: suspend: add noflush pushback")
> Signed-off-by: Bart Van Assche <bart.vanassche@sandisk.com>
> Cc: Kiyoshi Ueda <k-ueda@ct.jp.nec.com>

I did reinstate bio-based multipath recently.  But I just want to make
sure: you realize that dec_pending() is only ever used by bio-based DM
right? (dm-mq and .request_fn multipath are request-based)

For request-based DM's handling of DM_ENDIO_REQUEUE please see
drivers/md/dm-rq.c

> ---
>  drivers/md/dm.c | 16 +++++++++-------
>  1 file changed, 9 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/md/dm.c b/drivers/md/dm.c
> index fa9b1cb..6e04357 100644
> --- a/drivers/md/dm.c
> +++ b/drivers/md/dm.c
> @@ -767,6 +767,7 @@ static void dec_pending(struct dm_io *io, int error)
>  	int io_error;
>  	struct bio *bio;
>  	struct mapped_device *md = io->md;
> +	int noflush_suspending;
>  
>  	/* Push-back supersedes any I/O errors */
>  	if (unlikely(error)) {
> @@ -782,12 +783,16 @@ static void dec_pending(struct dm_io *io, int error)
>  			 * Target requested pushing back the I/O.
>  			 */
>  			spin_lock_irqsave(&md->deferred_lock, flags);
> -			if (__noflush_suspending(md))
> +			noflush_suspending = __noflush_suspending(md);
> +			if (noflush_suspending)
>  				bio_list_add_head(&md->deferred, io->bio);
> -			else
> -				/* noflush suspend was interrupted. */
> -				io->error = -EIO;
>  			spin_unlock_irqrestore(&md->deferred_lock, flags);
> +
> +			if (noflush_suspending)
> +				return;
> +
> +			/* noflush suspend was interrupted. */
> +			io->error = -EIO;
>  		}
>  
>  		io_error = io->error;
> @@ -795,9 +800,6 @@ static void dec_pending(struct dm_io *io, int error)
>  		end_io_acct(io);
>  		free_io(md, io);
>  
> -		if (io_error == DM_ENDIO_REQUEUE)
> -			return;
> -
>  		if ((bio->bi_opf & REQ_PREFLUSH) && bio->bi_iter.bi_size) {
>  			/*
>  			 * Preflush done for flush with data, reissue

bio-based DM's use of DM_ENDIO_REQUEUE is strictly confined to IFF the
target is performing no-flush suspend (in the mpath case, see
dm-mpath.c:must_push_back_bio()).  Otherwise, bio-based DM multipath
queues bios local to the DM multipath target (see dm-mapth.c's
'queued_bios' list).

NOTE: dm-cache-target.c's use of DM_ENDIO_REQUEUE isn't well gaurded to
only return DM_ENDIO_REQUEUE if the target is noflush suspending.  I'll
review this closer and also check with Joe.

--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel
Bart Van Assche - Aug. 26, 2016, 3:34 p.m.
On 08/26/2016 07:57 AM, Mike Snitzer wrote:
> On Thu, Aug 25 2016 at  4:57pm -0400,
> Bart Van Assche <bart.vanassche@sandisk.com> wrote:
>
>> Ensure that bio_endio() is called if io->error == DM_ENDIO_REQUEUE and
>> __noflush_suspending(md) returns false. Posting this as an RFC since I'm
>> not really familiar with the dm code.
>>
>> Fixes: commit 2e93ccc1933d ("dm: suspend: add noflush pushback")
>> Signed-off-by: Bart Van Assche <bart.vanassche@sandisk.com>
>> Cc: Kiyoshi Ueda <k-ueda@ct.jp.nec.com>
>
> I did reinstate bio-based multipath recently.  But I just want to make
> sure: you realize that dec_pending() is only ever used by bio-based DM
> right? (dm-mq and .request_fn multipath are request-based)

Hello Mike,

Yes, I am aware that this code is not triggered by my tests. This is 
something I noticed while reading the dm source code and something I 
wanted to report.

Bart.

--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel

Patch

diff --git a/drivers/md/dm.c b/drivers/md/dm.c
index fa9b1cb..6e04357 100644
--- a/drivers/md/dm.c
+++ b/drivers/md/dm.c
@@ -767,6 +767,7 @@  static void dec_pending(struct dm_io *io, int error)
 	int io_error;
 	struct bio *bio;
 	struct mapped_device *md = io->md;
+	int noflush_suspending;
 
 	/* Push-back supersedes any I/O errors */
 	if (unlikely(error)) {
@@ -782,12 +783,16 @@  static void dec_pending(struct dm_io *io, int error)
 			 * Target requested pushing back the I/O.
 			 */
 			spin_lock_irqsave(&md->deferred_lock, flags);
-			if (__noflush_suspending(md))
+			noflush_suspending = __noflush_suspending(md);
+			if (noflush_suspending)
 				bio_list_add_head(&md->deferred, io->bio);
-			else
-				/* noflush suspend was interrupted. */
-				io->error = -EIO;
 			spin_unlock_irqrestore(&md->deferred_lock, flags);
+
+			if (noflush_suspending)
+				return;
+
+			/* noflush suspend was interrupted. */
+			io->error = -EIO;
 		}
 
 		io_error = io->error;
@@ -795,9 +800,6 @@  static void dec_pending(struct dm_io *io, int error)
 		end_io_acct(io);
 		free_io(md, io);
 
-		if (io_error == DM_ENDIO_REQUEUE)
-			return;
-
 		if ((bio->bi_opf & REQ_PREFLUSH) && bio->bi_iter.bi_size) {
 			/*
 			 * Preflush done for flush with data, reissue