[PATCH] block: fix -EAGAIN IOPOLL task/vm accounting

[PATCH] block: fix -EAGAIN IOPOLL task/vm accounting

[Jens Axboe]

We currently increment the task/vm counts when we first attempt to queue a
bio. But this isn't necessarily correct - if the request allocation fails
with -EAGAIN, for example, and the caller retries, then we'll over-account
by as many retries as are done.

This can happen for polled IO, where we cannot wait for requests. Hence
retries can get aggressive, if we're running out of requests. If this
happens, then watching the IO rates in vmstat are incorrect as they count
every issue attempt as successful and hence the stats are inflated by
quite a lot potentially.

Add a bio flag to know if we've done accounting or not. This prevents
the same bio from being accounted potentially many times, when retried.

Signed-off-by: Jens Axboe <axboe@kernel.dk>

---

diff --git a/block/blk-core.c b/block/blk-core.c
index d9d632639bd1..ff562a8cd9c9 100644
--- a/block/blk-core.c
+++ b/block/blk-core.c
@@ -1236,7 +1236,7 @@ blk_qc_t submit_bio(struct bio *bio)
 	 * If it's a regular read/write or a barrier with data attached,
 	 * go through the normal accounting stuff before submission.
 	 */
-	if (bio_has_data(bio)) {
+	if (bio_has_data(bio) && !bio_flagged(bio, BIO_ACCOUNTED)) {
 		unsigned int count;
 
 		if (unlikely(bio_op(bio) == REQ_OP_WRITE_SAME))
@@ -1259,6 +1259,7 @@ blk_qc_t submit_bio(struct bio *bio)
 				(unsigned long long)bio->bi_iter.bi_sector,
 				bio_devname(bio, b), count);
 		}
+		bio_set_flag(bio, BIO_ACCOUNTED);
 	}
 
 	/*
diff --git a/include/linux/blk_types.h b/include/linux/blk_types.h
index 63a39e47fc60..39bcc9326c7a 100644
--- a/include/linux/blk_types.h
+++ b/include/linux/blk_types.h
@@ -266,6 +266,7 @@ enum {
 				 * of this bio. */
 	BIO_CGROUP_ACCT,	/* has been accounted to a cgroup */
 	BIO_TRACKED,		/* set if bio goes through the rq_qos path */
+	BIO_ACCOUNTED,		/* task/vm stats have been done */
 	BIO_FLAG_LAST
 };
 
-- 
Jens Axboe

Re: [PATCH] block: fix -EAGAIN IOPOLL task/vm accounting

[Christoph Hellwig]

On Sat, Aug 29, 2020 at 10:51:11AM -0600, Jens Axboe wrote:
> We currently increment the task/vm counts when we first attempt to queue a
> bio. But this isn't necessarily correct - if the request allocation fails
> with -EAGAIN, for example, and the caller retries, then we'll over-account
> by as many retries as are done.
> 
> This can happen for polled IO, where we cannot wait for requests. Hence
> retries can get aggressive, if we're running out of requests. If this
> happens, then watching the IO rates in vmstat are incorrect as they count
> every issue attempt as successful and hence the stats are inflated by
> quite a lot potentially.
> 
> Add a bio flag to know if we've done accounting or not. This prevents
> the same bio from being accounted potentially many times, when retried.

Can't the resubmitter just use submit_bio_noacct?  What is the call
stack here?

Re: [PATCH] block: fix -EAGAIN IOPOLL task/vm accounting

[Jens Axboe]

On 8/30/20 12:26 AM, Christoph Hellwig wrote:
> On Sat, Aug 29, 2020 at 10:51:11AM -0600, Jens Axboe wrote:
>> We currently increment the task/vm counts when we first attempt to queue a
>> bio. But this isn't necessarily correct - if the request allocation fails
>> with -EAGAIN, for example, and the caller retries, then we'll over-account
>> by as many retries as are done.
>>
>> This can happen for polled IO, where we cannot wait for requests. Hence
>> retries can get aggressive, if we're running out of requests. If this
>> happens, then watching the IO rates in vmstat are incorrect as they count
>> every issue attempt as successful and hence the stats are inflated by
>> quite a lot potentially.
>>
>> Add a bio flag to know if we've done accounting or not. This prevents
>> the same bio from being accounted potentially many times, when retried.
> 
> Can't the resubmitter just use submit_bio_noacct?  What is the call
> stack here?

The resubmitter is way higher than that. You could potentially have that
done in the block layer, but not higher up.

The use case is async submissions, going through ->read_iter() again.
Or ->write_iter().

-- 
Jens Axboe

Re: [PATCH] block: fix -EAGAIN IOPOLL task/vm accounting

[Christoph Hellwig]

On Sun, Aug 30, 2020 at 09:09:02AM -0600, Jens Axboe wrote:
> On 8/30/20 12:26 AM, Christoph Hellwig wrote:
> > On Sat, Aug 29, 2020 at 10:51:11AM -0600, Jens Axboe wrote:
> >> We currently increment the task/vm counts when we first attempt to queue a
> >> bio. But this isn't necessarily correct - if the request allocation fails
> >> with -EAGAIN, for example, and the caller retries, then we'll over-account
> >> by as many retries as are done.
> >>
> >> This can happen for polled IO, where we cannot wait for requests. Hence
> >> retries can get aggressive, if we're running out of requests. If this
> >> happens, then watching the IO rates in vmstat are incorrect as they count
> >> every issue attempt as successful and hence the stats are inflated by
> >> quite a lot potentially.
> >>
> >> Add a bio flag to know if we've done accounting or not. This prevents
> >> the same bio from being accounted potentially many times, when retried.
> > 
> > Can't the resubmitter just use submit_bio_noacct?  What is the call
> > stack here?
> 
> The resubmitter is way higher than that. You could potentially have that
> done in the block layer, but not higher up.
> 
> The use case is async submissions, going through ->read_iter() again.
> Or ->write_iter().

But how does a bio flag help there?  If we go through the file ops
again the next submission will be a new bio structure.

Re: [PATCH] block: fix -EAGAIN IOPOLL task/vm accounting

[Ming Lei]

On Sun, Aug 30, 2020 at 04:28:00PM +0100, Christoph Hellwig wrote:
> On Sun, Aug 30, 2020 at 09:09:02AM -0600, Jens Axboe wrote:
> > On 8/30/20 12:26 AM, Christoph Hellwig wrote:
> > > On Sat, Aug 29, 2020 at 10:51:11AM -0600, Jens Axboe wrote:
> > >> We currently increment the task/vm counts when we first attempt to queue a
> > >> bio. But this isn't necessarily correct - if the request allocation fails
> > >> with -EAGAIN, for example, and the caller retries, then we'll over-account
> > >> by as many retries as are done.
> > >>
> > >> This can happen for polled IO, where we cannot wait for requests. Hence
> > >> retries can get aggressive, if we're running out of requests. If this
> > >> happens, then watching the IO rates in vmstat are incorrect as they count
> > >> every issue attempt as successful and hence the stats are inflated by
> > >> quite a lot potentially.
> > >>
> > >> Add a bio flag to know if we've done accounting or not. This prevents
> > >> the same bio from being accounted potentially many times, when retried.
> > > 
> > > Can't the resubmitter just use submit_bio_noacct?  What is the call
> > > stack here?
> > 
> > The resubmitter is way higher than that. You could potentially have that
> > done in the block layer, but not higher up.
> > 
> > The use case is async submissions, going through ->read_iter() again.
> > Or ->write_iter().
> 
> But how does a bio flag help there?  If we go through the file ops
> again the next submission will be a new bio structure.

Yeah, we also have use cases of stack bio variable.



Thanks,
Ming

Re: [PATCH] block: fix -EAGAIN IOPOLL task/vm accounting

[Jens Axboe]

On 8/30/20 9:28 AM, Christoph Hellwig wrote:
> On Sun, Aug 30, 2020 at 09:09:02AM -0600, Jens Axboe wrote:
>> On 8/30/20 12:26 AM, Christoph Hellwig wrote:
>>> On Sat, Aug 29, 2020 at 10:51:11AM -0600, Jens Axboe wrote:
>>>> We currently increment the task/vm counts when we first attempt to queue a
>>>> bio. But this isn't necessarily correct - if the request allocation fails
>>>> with -EAGAIN, for example, and the caller retries, then we'll over-account
>>>> by as many retries as are done.
>>>>
>>>> This can happen for polled IO, where we cannot wait for requests. Hence
>>>> retries can get aggressive, if we're running out of requests. If this
>>>> happens, then watching the IO rates in vmstat are incorrect as they count
>>>> every issue attempt as successful and hence the stats are inflated by
>>>> quite a lot potentially.
>>>>
>>>> Add a bio flag to know if we've done accounting or not. This prevents
>>>> the same bio from being accounted potentially many times, when retried.
>>>
>>> Can't the resubmitter just use submit_bio_noacct?  What is the call
>>> stack here?
>>
>> The resubmitter is way higher than that. You could potentially have that
>> done in the block layer, but not higher up.
>>
>> The use case is async submissions, going through ->read_iter() again.
>> Or ->write_iter().
> 
> But how does a bio flag help there?  If we go through the file ops
> again the next submission will be a new bio structure.

Yeah the patch is garbage, can't work. The previous suggestion is here:

https://lore.kernel.org/linux-block/395b4c19-cc80-eebb-f6ab-04687110c84a@kernel.dk/T/

which isn't super pretty either, but at least it works. Not sure there's
a better solution, outside of marking the iocb as retry and then
carrying that flag forward for the bio as well. And that seems a bit
much for this case.

-- 
Jens Axboe

Re: [PATCH] block: fix -EAGAIN IOPOLL task/vm accounting

[Christoph Hellwig]

On Mon, Aug 31, 2020 at 08:02:43AM -0600, Jens Axboe wrote:
> >> The use case is async submissions, going through ->read_iter() again.
> >> Or ->write_iter().
> > 
> > But how does a bio flag help there?  If we go through the file ops
> > again the next submission will be a new bio structure.
> 
> Yeah the patch is garbage, can't work. The previous suggestion is here:
> 
> https://lore.kernel.org/linux-block/395b4c19-cc80-eebb-f6ab-04687110c84a@kernel.dk/T/
> 
> which isn't super pretty either, but at least it works. Not sure there's
> a better solution, outside of marking the iocb as retry and then
> carrying that flag forward for the bio as well. And that seems a bit
> much for this case.

We'll still need a flag with the above to skip the submit_bio_noacct
bios.  But I think it is the right way to go.  Eventually we'll also
need to push the accounting down into the individual bio based drivers.

Re: [PATCH] block: fix -EAGAIN IOPOLL task/vm accounting

[Jens Axboe]

On 8/31/20 8:12 AM, Christoph Hellwig wrote:
> On Mon, Aug 31, 2020 at 08:02:43AM -0600, Jens Axboe wrote:
>>>> The use case is async submissions, going through ->read_iter() again.
>>>> Or ->write_iter().
>>>
>>> But how does a bio flag help there?  If we go through the file ops
>>> again the next submission will be a new bio structure.
>>
>> Yeah the patch is garbage, can't work. The previous suggestion is here:
>>
>> https://lore.kernel.org/linux-block/395b4c19-cc80-eebb-f6ab-04687110c84a@kernel.dk/T/
>>
>> which isn't super pretty either, but at least it works. Not sure there's
>> a better solution, outside of marking the iocb as retry and then
>> carrying that flag forward for the bio as well. And that seems a bit
>> much for this case.
> 
> We'll still need a flag with the above to skip the submit_bio_noacct
> bios.  But I think it is the right way to go.  Eventually we'll also
> need to push the accounting down into the individual bio based drivers.

For the iocb propagation, we'd really need the caller to mark the iocb
as IOCB_ACCOUNTED (or whatever) if BIO_ACCOUNTED is set, since we can't
do that further down the stack as we really don't know if we hit -EAGAIN
before or after the bio was accounted... Which kind of sucks, as it'll
be hard to contain in a generic fashion.

-- 
Jens Axboe

Re: [PATCH] block: fix -EAGAIN IOPOLL task/vm accounting

[Christoph Hellwig]

On Mon, Aug 31, 2020 at 08:18:48AM -0600, Jens Axboe wrote:
> > We'll still need a flag with the above to skip the submit_bio_noacct
> > bios.  But I think it is the right way to go.  Eventually we'll also
> > need to push the accounting down into the individual bio based drivers.
> 
> For the iocb propagation, we'd really need the caller to mark the iocb
> as IOCB_ACCOUNTED (or whatever) if BIO_ACCOUNTED is set, since we can't
> do that further down the stack as we really don't know if we hit -EAGAIN
> before or after the bio was accounted... Which kind of sucks, as it'll
> be hard to contain in a generic fashion.

Well, that's why I think the only proper fix is to only account a bio
when we know the driver is actually going to submit it.

Re: [PATCH] block: fix -EAGAIN IOPOLL task/vm accounting

[Jens Axboe]

On 8/31/20 11:42 PM, Christoph Hellwig wrote:
> On Mon, Aug 31, 2020 at 08:18:48AM -0600, Jens Axboe wrote:
>>> We'll still need a flag with the above to skip the submit_bio_noacct
>>> bios.  But I think it is the right way to go.  Eventually we'll also
>>> need to push the accounting down into the individual bio based drivers.
>>
>> For the iocb propagation, we'd really need the caller to mark the iocb
>> as IOCB_ACCOUNTED (or whatever) if BIO_ACCOUNTED is set, since we can't
>> do that further down the stack as we really don't know if we hit -EAGAIN
>> before or after the bio was accounted... Which kind of sucks, as it'll
>> be hard to contain in a generic fashion.
> 
> Well, that's why I think the only proper fix is to only account a bio
> when we know the driver is actually going to submit it.

Yeah I agree, it's a lot less code too. Which is basically back to my
original RFC, I'll see if I can clean it up a bit.

-- 
Jens Axboe