Re: [PATCH V2 1/1] pmem: set QUEUE_FLAG_NOWAIT

Re: [PATCH V2 1/1] pmem: set QUEUE_FLAG_NOWAIT

[Christoph Hellwig]

Given that pmem simply loops over an arbitrarily large bio I think
we also need a threshold for which to allow nowait I/O.  While it
won't block for giant I/Os, doing all of them in the submitter
context isn't exactly the idea behind the nowait I/O.

I'm not really sure what a good theshold would be, though.

Btw, please also always add linux-block to the Cc list for block
driver patches that are even the slightest bit about the block
layer interface.

Re: [PATCH V2 1/1] pmem: set QUEUE_FLAG_NOWAIT

[Jeff Moyer]

Christoph Hellwig <hch@lst.de> writes:

> Given that pmem simply loops over an arbitrarily large bio I think
> we also need a threshold for which to allow nowait I/O.  While it
> won't block for giant I/Os, doing all of them in the submitter
> context isn't exactly the idea behind the nowait I/O.
>
> I'm not really sure what a good theshold would be, though.

There's no mention of the latency of the submission side in the
documentation for RWF_NOWAIT.  The man page says "Do not wait for data
which is not immediately available."  Data in pmem *is* immediately
available.  If we wanted to enforce a latency threshold for submission,
it would have to be configurable and, ideally, a part of the API.  I
don't think it's something we should even try to guarantee, though,
unless application writers are asking for it.

So, I think with the change to return -EAGAIN for writes to poisoned
memory, this patch is probably ok.

Chaitanya, could you send a v2?

Thanks,
Jeff

Re: [PATCH V2 1/1] pmem: set QUEUE_FLAG_NOWAIT

[Jens Axboe]

On 8/2/23 6:30?AM, Christoph Hellwig wrote:
> Given that pmem simply loops over an arbitrarily large bio I think
> we also need a threshold for which to allow nowait I/O.  While it
> won't block for giant I/Os, doing all of them in the submitter
> context isn't exactly the idea behind the nowait I/O.

You can do a LOT of looping over a giant bio and still come out way
ahead compared to needing to punt to a different thread. So I do think
it's the right choice. But I'm making assumptions here on what it looks
like, as I haven't seen the patch...

> Btw, please also always add linux-block to the Cc list for block
> driver patches that are even the slightest bit about the block
> layer interface.

Indeed. Particularly for these nowait changes, as some of them have been
pretty broken in the past.

-- 
Jens Axboe

Re: [PATCH V2 1/1] pmem: set QUEUE_FLAG_NOWAIT

[Christoph Hellwig]

On Wed, Aug 02, 2023 at 09:36:32AM -0600, Jens Axboe wrote:
> You can do a LOT of looping over a giant bio and still come out way
> ahead compared to needing to punt to a different thread. So I do think
> it's the right choice. But I'm making assumptions here on what it looks
> like, as I haven't seen the patch...

"a LOT" is relative.  A GB or two will block the submission thread for
quite a while.

Re: [PATCH V2 1/1] pmem: set QUEUE_FLAG_NOWAIT

[Chaitanya Kulkarni]

On 8/2/23 05:30, Christoph Hellwig wrote:
> Given that pmem simply loops over an arbitrarily large bio I think
> we also need a threshold for which to allow nowait I/O.  While it
> won't block for giant I/Os, doing all of them in the submitter
> context isn't exactly the idea behind the nowait I/O.
>
> I'm not really sure what a good theshold would be, though.
>
> Btw, please also always add linux-block to the Cc list for block
> driver patches that are even the slightest bit about the block
> layer interface.

will do ..

-ck

Re: [PATCH V2 1/1] pmem: set QUEUE_FLAG_NOWAIT

[Chaitanya Kulkarni]

On 8/2/23 08:36, Jens Axboe wrote:
> On 8/2/23 6:30?AM, Christoph Hellwig wrote:
>> Given that pmem simply loops over an arbitrarily large bio I think
>> we also need a threshold for which to allow nowait I/O.  While it
>> won't block for giant I/Os, doing all of them in the submitter
>> context isn't exactly the idea behind the nowait I/O.
> You can do a LOT of looping over a giant bio and still come out way
> ahead compared to needing to punt to a different thread. So I do think
> it's the right choice. But I'm making assumptions here on what it looks
> like, as I haven't seen the patch...
>

will send out the V3 soon and CC you and linux-block ...

>> Btw, please also always add linux-block to the Cc list for block
>> driver patches that are even the slightest bit about the block
>> layer interface.
> Indeed. Particularly for these nowait changes, as some of them have been
> pretty broken in the past.
>

yes, didn't forgot, lately dealing with bunch of stupidity, I'll send
zram and null_blk nowait changes soon with your comments fixed ..

-ck

Re: [PATCH V2 1/1] pmem: set QUEUE_FLAG_NOWAIT

[Chaitanya Kulkarni]

On 8/2/23 08:27, Jeff Moyer wrote:
> Christoph Hellwig <hch@lst.de> writes:
>
>> Given that pmem simply loops over an arbitrarily large bio I think
>> we also need a threshold for which to allow nowait I/O.  While it
>> won't block for giant I/Os, doing all of them in the submitter
>> context isn't exactly the idea behind the nowait I/O.
>>
>> I'm not really sure what a good theshold would be, though.
> There's no mention of the latency of the submission side in the
> documentation for RWF_NOWAIT.  The man page says "Do not wait for data
> which is not immediately available."  Data in pmem *is* immediately
> available.  If we wanted to enforce a latency threshold for submission,
> it would have to be configurable and, ideally, a part of the API.  I
> don't think it's something we should even try to guarantee, though,
> unless application writers are asking for it.

I need to see the usecase from application writter who cannot come
with a solution so kernel have to provide this interface, I'll be happy
to do that ..

> So, I think with the change to return -EAGAIN for writes to poisoned
> memory, this patch is probably ok.

I believe you mean the same one I've  provided earlier incremental ..

> Chaitanya, could you send a v2?

sure ...

-ck

Re: [PATCH V2 1/1] pmem: set QUEUE_FLAG_NOWAIT

[Jeff Moyer]

Chaitanya Kulkarni <chaitanyak@nvidia.com> writes:

> On 8/2/23 08:27, Jeff Moyer wrote:
>> So, I think with the change to return -EAGAIN for writes to poisoned
>> memory, this patch is probably ok.
>
> I believe you mean the same one I've  provided earlier incremental ..

Yes, sorry if that wasn't clear.

>> Chaitanya, could you send a v2?
>
> sure ...

and I guess I should have said v3.  ;-)

Cheers,
Jeff