On Wed, Dec 31, 2003 at 03:17:36AM -0800, Andrew Morton wrote:
> Andrew Morton <[email protected]> wrote:
> >
> > Let me actually think about this a bit.
>
> Nasty. The same race is present in 2.4.x...
>
> How's about we start new I/O in filemap_fdatawait() if the page is dirty?
>
Makes sense to me.
There's a chance that this could explain why Daniel saw exposures even
with his fix.
Would be interesting to see his results with your patch.
Though we might as well plug this anyway ?
>
> diff -puN mm/filemap.c~a mm/filemap.c
> --- 25/mm/filemap.c~a 2003-12-31 03:10:29.000000000 -0800
> +++ 25-akpm/mm/filemap.c 2003-12-31 03:17:05.000000000 -0800
> @@ -206,7 +206,13 @@ restart:
> page_cache_get(page);
> spin_unlock(&mapping->page_lock);
>
> - wait_on_page_writeback(page);
> + lock_page(page);
> + if (PageDirty(page) && mapping->a_ops->writepage) {
> + write_one_page(page, 1);
> + } else {
> + wait_on_page_writeback(page);
> + unlock_page(page);
Would we lose anything if we unlock_page() before wait_on_page_writeback() ?
I was thinking about the corresponding fix in sync_page_range, and it
would make life easier for retry based fs-AIO if we could move the
unlock_page before the wait.
> + }
> if (PageError(page))
> ret = -EIO;
>
>
>
Regards
Suparna
--
Suparna Bhattacharya ([email protected])
Linux Technology Center
IBM Software Lab, India
Suparna Bhattacharya <[email protected]> wrote:
>
> On Wed, Dec 31, 2003 at 03:17:36AM -0800, Andrew Morton wrote:
> > Andrew Morton <[email protected]> wrote:
> > >
> > > Let me actually think about this a bit.
> >
> > Nasty. The same race is present in 2.4.x...
> >
> > How's about we start new I/O in filemap_fdatawait() if the page is dirty?
> >
>
> Makes sense to me.
> There's a chance that this could explain why Daniel saw exposures even
> with his fix.
>
> Would be interesting to see his results with your patch.
>
> Though we might as well plug this anyway ?
Yes, we should. I'm not dreadfully keen on starting I/O from within
filemap_fdatawait() though - it seems "wrong" somehow.
> >
> > diff -puN mm/filemap.c~a mm/filemap.c
> > --- 25/mm/filemap.c~a 2003-12-31 03:10:29.000000000 -0800
> > +++ 25-akpm/mm/filemap.c 2003-12-31 03:17:05.000000000 -0800
> > @@ -206,7 +206,13 @@ restart:
> > page_cache_get(page);
> > spin_unlock(&mapping->page_lock);
> >
> > - wait_on_page_writeback(page);
> > + lock_page(page);
> > + if (PageDirty(page) && mapping->a_ops->writepage) {
> > + write_one_page(page, 1);
> > + } else {
> > + wait_on_page_writeback(page);
> > + unlock_page(page);
>
> Would we lose anything if we unlock_page() before wait_on_page_writeback() ?
No, that should be OK.
On Fri, 2 Jan 2004, Suparna Bhattacharya wrote:
> On Wed, Dec 31, 2003 at 03:17:36AM -0800, Andrew Morton wrote:
> > Andrew Morton <[email protected]> wrote:
> > >
> > > Let me actually think about this a bit.
> >
> > Nasty. The same race is present in 2.4.x...
filemap_fdatawait() is always called with i_sem held and
there is no "!PG_dirty and !PG_writeback" window.
Where does the race lies in 2.4 ?
Daniel, Would be interesting to know if the direct IO tests also fail on
2.4.
> > How's about we start new I/O in filemap_fdatawait() if the page is
> > dirty?
Marcelo Tosatti <[email protected]> wrote:
>
>
>
> On Fri, 2 Jan 2004, Suparna Bhattacharya wrote:
>
> > On Wed, Dec 31, 2003 at 03:17:36AM -0800, Andrew Morton wrote:
> > > Andrew Morton <[email protected]> wrote:
> > > >
> > > > Let me actually think about this a bit.
> > >
> > > Nasty. The same race is present in 2.4.x...
>
> filemap_fdatawait() is always called with i_sem held and
> there is no "!PG_dirty and !PG_writeback" window.
>
> Where does the race lies in 2.4 ?
kupdate and bdflush run filemap_fdatawait() without i_sem. If kupdate has
moved a page off locked_pages to wait on it, a caller of fsync() just won't
see that page at all, and fsync can return while I/O is still in flight.
Given that this only applies to mmap data in 2.4 it doesn't seem
super-important. A good way to fix it would be to not call
filemap_fdatawait() *at all* if the caller is kupdate or bdflush - it's
silly. Could use a PF_foo flag for this?
On Fri, 2 Jan 2004, Suparna Bhattacharya wrote:
> On Wed, Dec 31, 2003 at 03:17:36AM -0800, Andrew Morton wrote:
> > Andrew Morton <[email protected]> wrote:
> > >
> > > Let me actually think about this a bit.
> >
> > Nasty. The same race is present in 2.4.x...
filemap_fdatawait() is always called with i_sem held and
there is no "!PG_dirty and !PG_writeback" window.
Where does the race lies in 2.4 ?
Daniel, Would be interesting to know if the direct IO tests also fail on
2.4.
> > How's about we start new I/O in filemap_fdatawait() if the page is
> > dirty?