Subject: Re: [PATCH] aio: Add command to wait completion of all requests
To: Benjamin LaHaise <bcrl@kvack.org>
References: <149700173837.15252.8419518498235874341.stgit@localhost.localdomain>
 <20170613144248.GK7230@kvack.org>
 <4637a41e-b9af-a685-f4af-f3ca2a287a1e@virtuozzo.com>
 <20170613152601.GM7230@kvack.org>
 <02671cea-6518-d87b-61f8-8583cb1ffc90@virtuozzo.com>
 <20170613232645.GT7230@kvack.org>
Cc: avagin@openvz.org, linux-kernel@vger.kernel.org,
        viro@zeniv.linux.org.uk, gorcunov@openvz.org,
        akpm@linux-foundation.org, xemul@virtuozzo.com
From: Kirill Tkhai <ktkhai@virtuozzo.com>
Message-ID: <8bc01429-0041-523f-8b36-d30c7b84408a@virtuozzo.com>
Date: Wed, 14 Jun 2017 12:11:38 +0300
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101
 Thunderbird/45.8.0
MIME-Version: 1.0
In-Reply-To: <20170613232645.GT7230@kvack.org>
Content-Type: text/plain; charset=windows-1252
Content-Transfer-Encoding: 7bit
SpamDiagnosticOutput: 1:99
SpamDiagnosticMetadata: NSPM
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 14 Jun 2017 09:12:00.4295 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-Transport-CrossTenantHeadersStamped: VI1PR0801MB1341
Sender: linux-kernel-owner@vger.kernel.org
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org
Content-Length: 3830
Lines: 76

On 14.06.2017 02:26, Benjamin LaHaise wrote:
> On Tue, Jun 13, 2017 at 07:17:43PM +0300, Kirill Tkhai wrote:
>> On 13.06.2017 18:26, Benjamin LaHaise wrote:
>>> On Tue, Jun 13, 2017 at 06:11:03PM +0300, Kirill Tkhai wrote:
>>> ...
>>>> The functionality, I did, grew from real need and experience. We try to
>>>> avoid kernel modification, where it's possible, but the in-flight aio
>>>> requests is not a case suitable for that.
>>>
>>> What you've done only works for *your* use-case, but not in general.  Like
>>> in other subsystems, you need to provide hooks on a per file descriptor
>>> basis for quiescing different kinds of file descriptors.
>>
>> Which hooks do you suggest? It's possible there is no a file descriptor open after
>> a request is submitted. Do you suggest an interface to reopen a struct file?
> 
> That's what you have to contend with.  AIO keeps the struct file pinned
> for the life of the request.  This is one of the complex issues you need
> to address.
> 
>>> Your current
>>> patch set completely ignores things like usb gadget.  You need to create
>>> infrastructure for restarting i/os after your checkpointing occurs, which
>>> you haven't put any thought into in this patchset.  If you want to discuss
>>> how to do that, fine, but the approach in this patchset simply does not
>>> work in general.  What happens when an aio doesn't complete or takes hours
>>> to complete?
>>
>> Here is wait_event_interruptible(), but it's possible to convert it
>> in wait_event_interruptible_hrtimeout() like it's made in read_events().
>> It's not a deadly issue of patch. The function read_events() simply
>> waits for timeout, can't we do the same?
> 
> Nope.  An aio may not complete in a timely fashion, in which case your
> wait for all aios to complete will simply wait forever.  I take it this is
> not the desired behaviour of checkpointing.

But read_events() does it. What the difference between them?
 
>> Could you please describe how will cancelling aio requests will help to wait
>> till their completion? Is there is guarantee, they will be easily queued back?
>> I suppose, no, because there are may be a memory limit or some low level
>> drivers limitations, dependent on internal conditions.
> 
> This is the problem you're facing.  Quiescing aios is complicated!

It's too generic words and they may refer to anything. And it's a problem,
which is not connected with small aio engine, because it's impossible to guarantee
availability of kernel resources from there. Imagine, parallel task eats memory
of your just cancelled request: then you just can't queue request back.

This argument makes your suggestion not rock-stable suitable for all cases:
it will break user applications in such situations.

>> Also, it's not seems good to overload aio with the functionality of obtaining
>> closed file descriptors of submitted requests.
>>
>> Do you mean this way, or I misunderstood you? Could you please to concretize your
>> idea?
> 
> Yes, this is what I'm talking about.
> 
>> In my vision cancelling requests does not allow to implement the need I described.
>> If we can't queue request back, it breaks snapshotting and user application in
>> general.
> 
> This is what you have to figure out.  This is why your patch is incomplete
> and cannot be accepted.  You can't punt the complexity your feature
> requires onto other maintainers -- your implementation has to be reasonably
> complete at time of patch inclusion.  Can you see now why your patch can't
> be included as-is?  The assumption you made that aios complete in a timely
> fashion is incorrect.  Everything else stems from that.

I just can't see why read_events() just waits for requests completion not paying
attention of the all above you said. Could you please clarify the difference
between two these situations?