Re: [PATCH 2/4] fs: add support for LOOKUP_NONBLOCK

[Jens Axboe <axboe@kernel.dk>]

On 12/15/20 9:08 AM, Jens Axboe wrote:
> On 12/15/20 8:37 AM, Jens Axboe wrote:
>> On 12/15/20 8:33 AM, Matthew Wilcox wrote:
>>> On Tue, Dec 15, 2020 at 08:29:40AM -0700, Jens Axboe wrote:
>>>> On 12/15/20 5:24 AM, Matthew Wilcox wrote:
>>>>> On Mon, Dec 14, 2020 at 12:13:22PM -0700, Jens Axboe wrote:
>>>>>> +++ b/fs/namei.c
>>>>>> @@ -686,6 +686,8 @@ static bool try_to_unlazy(struct nameidata *nd)
>>>>>>  	BUG_ON(!(nd->flags & LOOKUP_RCU));
>>>>>>  
>>>>>>  	nd->flags &= ~LOOKUP_RCU;
>>>>>> +	if (nd->flags & LOOKUP_NONBLOCK)
>>>>>> +		goto out1;
>>>>>
>>>>> If we try a walk in a non-blocking context, it fails, then we punt to
>>>>> a thread, do we want to prohibit that thread trying an RCU walk first?
>>>>> I can see arguments both ways -- this may only be a temporary RCU walk
>>>>> failure, or we may never be able to RCU walk this path.
>>>>
>>>> In my opinion, it's not worth it trying to over complicate matters by
>>>> handling the retry side differently. Better to just keep them the
>>>> same. We'd need a lookup anyway to avoid aliasing.
>>>
>>> but by clearing LOOKUP_RCU here, aren't you making the retry handle
>>> things differently?  maybe i got lost.
>>
>> That's already how it works, I'm just clearing LOOKUP_NONBLOCK (which
>> relies on LOOKUP_RCU) when we're clearing LOOKUP_RCU. I can try and
>> benchmark skipping LOOKUP_RCU when we do the blocking retry, but my gut
>> tells me it'll be noise.
> 
> OK, ran some numbers. The test app benchmarks opening X files, I just
> used /usr on my test box. That's 182677 files. To mimic real worldy
> kind of setups, 33% of the files can be looked up hot, so LOOKUP_NONBLOCK
> will succeed.
> 
> Patchset as posted:
> 
> Method		Time (usec)
> ---------------------------
> openat		2,268,930
> openat		2,274,256
> openat		2,274,256
> io_uring	  917,813
> io_uring	  921,448 
> io_uring	  915,233
> 
> And with a LOOKUP_NO_RCU flag, which io_uring sets when it has to do
> retry, and which will make namei skip the first LOOKUP_RCU for path
> resolution:
> 
> Method		Time (usec)
> ---------------------------
> io_uring	  902,410
> io_uring	  902,725
> io_uring	  896,289
> 
> Definitely not faster - whether that's just reboot noise, or if it's
> significant, I'd need to look deeper to figure out.

If you're puzzled by the conclusion based on the numbers, there's a good
reason. The first table is io_uring + LOOKUP_NO_RCU for retry, second
table is io_uring as posted. I mistakenly swapped the numbers around...

So conclusion still stands, I just pasted in the wrong set for the
table.

-- 
Jens Axboe