Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <a06a92ed-2b22-94a4-f1df-743303da2f3a@kernel.dk>
Date:   Wed, 28 Jun 2023 19:29:33 -0600
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux aarch64; rv:102.0) Gecko/20100101
 Thunderbird/102.12.0
Subject: Re: [GIT PULL] bcachefs
Content-Language: en-US
To:     Kent Overstreet <kent.overstreet@linux.dev>
Cc:     torvalds@linux-foundation.org, linux-kernel@vger.kernel.org,
        linux-fsdevel@vger.kernel.org, linux-bcachefs@vger.kernel.org,
        Christoph Hellwig <hch@lst.de>,
        Christian Brauner <brauner@kernel.org>,
        Al Viro <viro@zeniv.linux.org.uk>
References: <20230628040114.oz46icbsjpa4egpp@moria.home.lan>
 <b02657af-5bbb-b46b-cea0-ee89f385f3c1@kernel.dk>
 <4b863e62-4406-53e4-f96a-f4d1daf098ab@kernel.dk>
 <20230628175204.oeek4nnqx7ltlqmg@moria.home.lan>
 <e1570c46-68da-22b7-5322-f34f3c2958d9@kernel.dk>
 <2e635579-37ba-ddfc-a2ab-e6c080ab4971@kernel.dk>
 <20230628221342.4j3gr3zscnsu366p@moria.home.lan>
 <d697ec27-8008-2eb6-0950-f612a602dcf5@kernel.dk>
 <20230628225514.n3xtlgmjkgapgnrd@moria.home.lan>
 <1e2134f1-f48b-1459-a38e-eac9597cd64a@kernel.dk>
 <20230628235018.ttvtzpfe42fri4yq@moria.home.lan>
From:   Jens Axboe <axboe@kernel.dk>
In-Reply-To: <20230628235018.ttvtzpfe42fri4yq@moria.home.lan>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
Precedence: bulk

On 6/28/23 5:50?PM, Kent Overstreet wrote:
> On Wed, Jun 28, 2023 at 05:14:09PM -0600, Jens Axboe wrote:
>> On 6/28/23 4:55?PM, Kent Overstreet wrote:
>>>> But it's not aio (or io_uring or whatever), it's simply the fact that
>>>> doing an fput() from an exiting task (for example) will end up being
>>>> done async. And hence waiting for task exits is NOT enough to ensure
>>>> that all file references have been released.
>>>>
>>>> Since there are a variety of other reasons why a mount may be pinned and
>>>> fail to umount, perhaps it's worth considering that changing this
>>>> behavior won't buy us that much. Especially since it's been around for
>>>> more than 10 years:
>>>
>>> Because it seems that before io_uring the race was quite a bit harder to
>>> hit - I only started seeing it when things started switching over to
>>> io_uring. generic/388 used to pass reliably for me (pre backpointers),
>>> now it doesn't.
>>
>> I literally just pasted a script that hits it in one second with aio. So
>> maybe generic/388 doesn't hit it as easily, but it's surely TRIVIAL to
>> hit with aio. As demonstrated. The io_uring is not hard to bring into
>> parity on that front, here's one I posted earlier today for 6.5:
>>
>> https://lore.kernel.org/io-uring/20230628170953.952923-4-axboe@kernel.dk/
>>
>> Doesn't change the fact that you can easily hit this with io_uring or
>> aio, and probably more things too (didn't look any further). Is it a
>> realistic thing outside of funky tests? Probably not really, or at least
>> if those guys hit it they'd probably have the work-around hack in place
>> in their script already.
>>
>> But the fact is that it's been around for a decade. It's somehow a lot
>> easier to hit with bcachefs than XFS, which may just be because the
>> former has a bunch of workers and this may be deferring the delayed fput
>> work more. Just hand waving.
> 
> Not sure what you're arguing here...?
> 
> We've had a long standing bug, it's recently become much easier to hit
> (for multiple reasons); we seem to be in agreement on all that. All I'm
> saying is that the existence of that bug previously is not reason to fix
> it now.

Not really arguing, just stating that it's not a huge problem as it's
not something that real world would tend to do and probably why we saw
it in a test case instead.

>>>> then we'd probably want to move that deferred fput list to the
>>>> task_struct and ensure that it gets run if the task exits rather than
>>>> have a global deferred list. Currently we have:
>>>>
>>>>
>>>> 1) If kthread or in interrupt
>>>> 	1a) add to global fput list
>>>> 2) task_work_add if not. If that fails, goto 1a.
>>>>
>>>> which would then become:
>>>>
>>>> 1) If kthread or in interrupt
>>>> 	1a) add to global fput list
>>>> 2) task_work_add if not. If that fails, we know task is existing. add to
>>>>    per-task defer list to be run at a convenient time before task has
>>>>    exited.
>>>
>>> no, it becomes:
>>>  if we're running in a user task, or if we're doing an operation on
>>>  behalf of a user task, add to the user task's deferred list: otherwise
>>>  add to global deferred list.
>>
>> And how would the "on behalf of a user task" work in terms of being
>> in_interrupt()?
> 
> I don't see any relation to in_interrupt?

Just saying that you'd now need the task passed in.

> We'd have to add a version of fput() that takes an additional
> task_struct argument, and plumb that through the aio code - kioctx
> lifetime is tied to mm_struct, not task_struct, so we'd have to add a
> ref to the task_struct to kiocb.
> 
> Which would probably be a good thing tbh, it'd let us e.g. account cpu
> time back to the original task when kiocb completion has to run out of a
> workqueue.

Might also introduce some funky dependencies. Probably not an issue it
tied to the aio_kiocb. If you go ahead with that, just make sure you
keep the task referencing out of the fput variant for users that don't
need that.

-- 
Jens Axboe