All of lore.kernel.org
 help / color / mirror / Atom feed
From: Zach Brown <zach.brown@oracle.com>
To: suparna@in.ibm.com
Cc: Jiri Kosina <jikos@jikos.cz>,
	Andrew Morton <akpm@linux-foundation.org>,
	"Ananiev, Leonid I" <leonid.i.ananiev@intel.com>,
	linux-kernel@vger.kernel.org, linux-aio <linux-aio@kvack.org>,
	Chris Mason <chris.mason@oracle.com>,
	Badari Pulavarty <pbadari@us.ibm.com>, Jan Kara <jack@suse.cz>
Subject: Re: [PATCH] aio: fix kernel bug when page is  temporally busy
Date: Fri, 9 Feb 2007 12:02:08 -0500	[thread overview]
Message-ID: <5C55DFF2-6593-41FD-A161-D99331E419E3@oracle.com> (raw)
In-Reply-To: <20070209110555.GA11232@in.ibm.com>


On Feb 9, 2007, at 6:05 AM, Suparna Bhattacharya wrote:

> On Fri, Feb 09, 2007 at 11:40:27AM +0100, Jiri Kosina wrote:
>> On Fri, 9 Feb 2007, Andrew Morton wrote:
>>
>>>> @@ -1204,7 +1204,7 @@ generic_file_aio_read(struct kiocb *iocb,  
>>>> const struct iovec *iov,
>>>>  			do_generic_file_read(filp,ppos,&desc,file_read_actor);
>>>>  			retval += desc.written;
>>>>  			if (desc.error) {
>>>> -				retval = retval ?: desc.error;
>>>> +				retval = desc.error;

I was worried about this too.

> blocking. The high level AIO code (see aio_rw_vect_rety) has the  
> ability
> to handle this.

I had missed this, and yeah, that's some level of comfort.

But I'm not convinced we can guarantee that's safe.  The positive  
return code that aio_rw_vect_retry() sees is telling it that some IO  
has completed and, arguably, that no more IO is in flight.  If we  
return partial progress from generic_file_aio_read() while we have an  
iocb in a wait queue then we are adding yet another invariant.  That  
while an iocb is pending from a previous call down the call chain, we  
can't return a non-aio negative error.  Doing so would cause fs/aio.c  
to complete while there is still an iocb on a wait queue from a  
previous retry attempt.  Right?

I also noticed something just now while poking around these paths to  
see if I could get the start of generic_file_aio_read() to fail when  
it had previously succeeded.  What's to stop another task from racing  
to set O_DIRECT between retries?

That sounds like a pretty hilarious way to get a read retry to fail  
due to buffer misalignment while a previously buffered instance of it  
is still in flight.  Hi-larious.

In thinking about this a discussing it with Chris a bit, I wonder if  
the right fix isn't to refuse changing O_DIRECT via setfl() once any  
IO paths have started on the filp.  Something like:

	filp->frozen_flags |= O_DIRECT

at the start of paths and check it in setfl()?

Are we similarly worried about O_APPEND?

- z

  parent reply	other threads:[~2007-02-09 17:03 UTC|newest]

Thread overview: 31+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2007-02-09  4:29 [PATCH] aio: fix kernel bug when page is temporally busy Ananiev, Leonid I
2007-02-09  4:35 ` Andrew Morton
2007-02-09  5:41   ` Ananiev, Leonid I
2007-02-09  5:52     ` Andrew Morton
2007-02-12 22:52       ` Ananiev, Leonid I
2007-02-12 23:21       ` Ananiev, Leonid I
2007-02-09  7:16     ` Suparna Bhattacharya
2007-02-09  9:52       ` Ananiev, Leonid I
2007-02-09 10:11         ` Jiri Kosina
2007-02-10 18:05         ` Ken Chen
2007-02-10 18:17           ` Ananiev, Leonid I
2007-02-10 18:27           ` Ananiev, Leonid I
2007-02-10 21:57           ` Ananiev, Leonid I
2007-02-15  9:16           ` Ananiev, Leonid I
2007-02-15 18:25             ` Zach Brown
2007-02-15 19:11               ` Ananiev, Leonid I
2007-02-15 19:22                 ` Zach Brown
2007-02-15 21:06                   ` Ananiev, Leonid I
2007-02-15 23:32                   ` Ananiev, Leonid I
2007-02-16  0:01                     ` Zach Brown
2007-02-16 12:18                       ` Ananiev, Leonid I
2007-02-09  9:54 ` Jiri Kosina
2007-02-09 10:14   ` Andrew Morton
2007-02-09 10:40     ` Jiri Kosina
2007-02-09 11:05       ` Suparna Bhattacharya
2007-02-09 11:18         ` Ananiev, Leonid I
2007-02-09 17:02         ` Zach Brown [this message]
2007-02-10 19:36 Ananiev, Leonid I
2007-02-14 17:51 Ananiev, Leonid I
2007-02-15  3:30 ` Andrew Morton
2007-02-15  5:26   ` Ananiev, Leonid I

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=5C55DFF2-6593-41FD-A161-D99331E419E3@oracle.com \
    --to=zach.brown@oracle.com \
    --cc=akpm@linux-foundation.org \
    --cc=chris.mason@oracle.com \
    --cc=jack@suse.cz \
    --cc=jikos@jikos.cz \
    --cc=leonid.i.ananiev@intel.com \
    --cc=linux-aio@kvack.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=pbadari@us.ibm.com \
    --cc=suparna@in.ibm.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.