Re: [PATCH 1/3] io_uring: add support for async work inheriting files table

From: Jens Axboe <axboe@kernel.dk>
To: Jann Horn <jannh@google.com>
Cc: linux-block@vger.kernel.org,
	"David S. Miller" <davem@davemloft.net>,
	Network Development <netdev@vger.kernel.org>
Subject: Re: [PATCH 1/3] io_uring: add support for async work inheriting files table
Date: Thu, 24 Oct 2019 13:41:54 -0600	[thread overview]
Message-ID: <c3fb07d4-223c-8835-5c22-68367e957a4f@kernel.dk> (raw)
In-Reply-To: <CAG48ez1akvnVpK3dMH4H=C2CsNGDZkDaxZEF2stGAPCnUcaa+g@mail.gmail.com>

On 10/18/19 12:50 PM, Jann Horn wrote:
> On Fri, Oct 18, 2019 at 8:16 PM Jens Axboe <axboe@kernel.dk> wrote:
>> On 10/18/19 12:06 PM, Jann Horn wrote:
>>> But actually, by the way: Is this whole files_struct thing creating a
>>> reference loop? The files_struct has a reference to the uring file,
>>> and the uring file has ACCEPT work that has a reference to the
>>> files_struct. If the task gets killed and the accept work blocks, the
>>> entire files_struct will stay alive, right?
>>
>> Yes, for the lifetime of the request, it does create a loop. So if the
>> application goes away, I think you're right, the files_struct will stay.
>> And so will the io_uring, for that matter, as we depend on the closing
>> of the files to do the final reap.
>>
>> Hmm, not sure how best to handle that, to be honest. We need some way to
>> break the loop, if the request never finishes.
> 
> A wacky and dubious approach would be to, instead of taking a
> reference to the files_struct, abuse f_op->flush() to synchronously
> flush out pending requests with references to the files_struct... But
> it's probably a bad idea, given that in f_op->flush(), you can't
> easily tell which files_struct the close is coming from. I suppose you
> could keep a list of (fdtable, fd) pairs through which ACCEPT requests
> have come in and then let f_op->flush() probe whether the file
> pointers are gone from them...

Got back to this after finishing the io-wq stuff, which we need for the
cancel.

Here's an updated patch:

http://git.kernel.dk/cgit/linux-block/commit/?h=for-5.5/io_uring-test&id=1ea847edc58d6a54ca53001ad0c656da57257570

that seems to work for me (lightly tested), we correctly find and cancel
work that is holding on to the file table.

The full series sits on top of my for-5.5/io_uring-wq branch, and can be
viewed here:

http://git.kernel.dk/cgit/linux-block/log/?h=for-5.5/io_uring-test

Let me know what you think!

-- 
Jens Axboe