Linux-Block Archive on lore.kernel.org
 help / color / Atom feed
From: Damien Le Moal <Damien.LeMoal@wdc.com>
To: Tetsuo Handa <penguin-kernel@i-love.sakura.ne.jp>,
	Bob Liu <bob.liu@oracle.com>, "axboe@kernel.dk" <axboe@kernel.dk>
Cc: "linux-block@vger.kernel.org" <linux-block@vger.kernel.org>
Subject: Re: [PATCH] block: Bail out iteration functions upon SIGKILL.
Date: Wed, 13 Nov 2019 01:54:14 +0000
Message-ID: <BYAPR04MB5816D18E6F6633030265B06EE7760@BYAPR04MB5816.namprd04.prod.outlook.com> (raw)
In-Reply-To: <272e3542-72ab-12ff-636b-722a68a2589c@i-love.sakura.ne.jp>

On 2019/11/12 23:48, Tetsuo Handa wrote:
[...]
>>> +static int blk_should_abort(struct bio *bio)
>>> +{
>>> +	int ret;
>>> +
>>> +	cond_resched();
>>> +	if (!fatal_signal_pending(current))
>>> +		return 0;
>>> +	ret = submit_bio_wait(bio);
>>
>> This will change the behavior of __blkdev_issue_discard() to a sync IO
>> execution instead of the current async execution since submit_bio_wait()
>> call is the responsibility of the caller (e.g. blkdev_issue_discard()).
>> Have you checked if users of __blkdev_issue_discard() are OK with that ?
>> f2fs, ext4, xfs, dm and nvme use this function.
> 
> I'm not sure...
> 
>>
>> Looking at f2fs, this does not look like it is going to work as expected
>> since the bio setup, including end_io callback, is done after this
>> function is called and a regular submit_bio() execution is being used.
> 
> Then, just breaking the iteration like below?
> nvmet_bdev_execute_write_zeroes() ignores -EINTR if "*biop = bio;" is done. Is that no problem?
> 
> --- a/block/blk-lib.c
> +++ b/block/blk-lib.c
> @@ -7,6 +7,7 @@
>  #include <linux/bio.h>
>  #include <linux/blkdev.h>
>  #include <linux/scatterlist.h>
> +#include <linux/sched/signal.h>
>  
>  #include "blk.h"
>  
> @@ -30,6 +31,7 @@ int __blkdev_issue_discard(struct block_device *bdev, sector_t sector,
>  	struct bio *bio = *biop;
>  	unsigned int op;
>  	sector_t bs_mask;
> +	int ret = 0;
>  
>  	if (!q)
>  		return -ENXIO;
> @@ -76,10 +78,14 @@ int __blkdev_issue_discard(struct block_device *bdev, sector_t sector,
>  		 * is disabled.
>  		 */
>  		cond_resched();
> +		if (fatal_signal_pending(current)) {
> +			ret = -EINTR;
> +			break;
> +		}
>  	}
>  
>  	*biop = bio;
> -	return 0;
> +	return ret;

This will leak a bio as blkdev_issue_discard() executes the bio only in
the case "if (!ret && bio)". So that does not work as is, unless all
callers of __blkdev_issue_discard() are also changed. Same problem for
the other __blkdev_issue_xxx() functions.

Looking more into this, if an error is returned here, no bio should be
returned and we need to make sure that all started bios are also
completed. So your helper blk_should_abort() did the right thing calling
submit_bio_wait(). However, I Think it would be better to fail
immediately the current loop bio instead of executing it and then
reporting the -EINTR error, unconditionally, regardless of what the
started bios completion status is.

This could be done with the help of a function like this, very similar
to submit_bio_wait().

void bio_chain_end_wait(struct bio *bio)
{
	DECLARE_COMPLETION_ONSTACK_MAP(done, bio->bi_disk->lockdep_map);

	bio->bi_private = &done;
	bio->bi_end_io = submit_bio_wait_endio;
	bio->bi_opf |= REQ_SYNC;
	bio_endio(bio);
	wait_for_completion_io(&done);
}

And then your helper function becomes something like this:

static int blk_should_abort(struct bio *bio)
{
	int ret;

	cond_resched();
	if (!fatal_signal_pending(current))
		return 0;

	if (bio_flagged(bio, BIO_CHAIN))
		bio_chain_end_wait(bio);
	bio_put(bio);

	return -EINTR;
}

Thoughts ?


>  }
>  EXPORT_SYMBOL(__blkdev_issue_discard);
>  
> @@ -136,6 +142,7 @@ static int __blkdev_issue_write_same(struct block_device *bdev, sector_t sector,
>  	unsigned int max_write_same_sectors;
>  	struct bio *bio = *biop;
>  	sector_t bs_mask;
> +	int ret = 0;
>  
>  	if (!q)
>  		return -ENXIO;
> @@ -172,10 +179,14 @@ static int __blkdev_issue_write_same(struct block_device *bdev, sector_t sector,
>  			nr_sects = 0;
>  		}
>  		cond_resched();
> +		if (fatal_signal_pending(current)) {
> +			ret = -EINTR;
> +			break;
> +		}
>  	}
>  
>  	*biop = bio;
> -	return 0;
> +	return ret;
>  }
>  
>  /**
> @@ -216,6 +227,7 @@ static int __blkdev_issue_write_zeroes(struct block_device *bdev,
>  	struct bio *bio = *biop;
>  	unsigned int max_write_zeroes_sectors;
>  	struct request_queue *q = bdev_get_queue(bdev);
> +	int ret = 0;
>  
>  	if (!q)
>  		return -ENXIO;
> @@ -246,10 +258,14 @@ static int __blkdev_issue_write_zeroes(struct block_device *bdev,
>  			nr_sects = 0;
>  		}
>  		cond_resched();
> +		if (fatal_signal_pending(current)) {
> +			ret = -EINTR;
> +			break;
> +		}
>  	}
>  
>  	*biop = bio;
> -	return 0;
> +	return ret;
>  }
>  
>  /*
> @@ -273,6 +289,7 @@ static int __blkdev_issue_zero_pages(struct block_device *bdev,
>  	struct bio *bio = *biop;
>  	int bi_size = 0;
>  	unsigned int sz;
> +	int ret = 0;
>  
>  	if (!q)
>  		return -ENXIO;
> @@ -296,10 +313,14 @@ static int __blkdev_issue_zero_pages(struct block_device *bdev,
>  				break;
>  		}
>  		cond_resched();
> +		if (fatal_signal_pending(current)) {
> +			ret = -EINTR;
> +			break;
> +		}
>  	}
>  
>  	*biop = bio;
> -	return 0;
> +	return ret;
>  }
>  
>  /**
> 
>>
>>> +	bio_put(bio);
>>> +	return ret ? ret : -EINTR;
>>> +}
>>> +
>>>  struct bio *blk_next_bio(struct bio *bio, unsigned int nr_pages, gfp_t gfp)
>>>  {
>>>  	struct bio *new = bio_alloc(gfp, nr_pages);
> 


-- 
Damien Le Moal
Western Digital Research

  reply index

Thread overview: 14+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-10-23  7:56 INFO: task syz-executor can't die for more than 143 seconds. (2) syzbot
2019-10-24 10:08 ` Tetsuo Handa
2019-10-28  8:51   ` Bob Liu
2019-11-08 11:41     ` [PATCH] block: Bail out iteration functions upon SIGKILL Tetsuo Handa
2019-11-08 18:13       ` Chaitanya Kulkarni
2019-11-08 22:18         ` Chaitanya Kulkarni
2019-11-12  4:05       ` Damien Le Moal
2019-11-12 14:47         ` Tetsuo Handa
2019-11-13  1:54           ` Damien Le Moal [this message]
2019-11-13  6:55             ` Ming Lei
2019-11-13  7:11               ` Damien Le Moal
2019-11-13  7:49                 ` Ming Lei
2019-11-15 10:05             ` Tetsuo Handa
2019-11-18  0:02               ` Damien Le Moal

Reply instructions:

You may reply publically to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=BYAPR04MB5816D18E6F6633030265B06EE7760@BYAPR04MB5816.namprd04.prod.outlook.com \
    --to=damien.lemoal@wdc.com \
    --cc=axboe@kernel.dk \
    --cc=bob.liu@oracle.com \
    --cc=linux-block@vger.kernel.org \
    --cc=penguin-kernel@i-love.sakura.ne.jp \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Linux-Block Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-block/0 linux-block/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-block linux-block/ https://lore.kernel.org/linux-block \
		linux-block@vger.kernel.org
	public-inbox-index linux-block

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-block


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git