linux-ext4 - Re: [PATCH] jbd_commit_transaction() races with journal_try_to_drop

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-Id: <1210786872.3657.48.camel@localhost.localdomain>
Date:	Wed, 14 May 2008 10:41:12 -0700
From:	Mingming Cao <cmm@...ibm.com>
To:	Jan Kara <jack@...e.cz>
Cc:	Badari Pulavarty <pbadari@...ibm.com>, akpm@...ux-foundation.org,
	linux-ext4@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] jbd_commit_transaction() races with
	journal_try_to_drop_buffers() causing DIO failures

On Wed, 2008-05-14 at 19:08 +0200, Jan Kara wrote:
> On Tue 13-05-08 15:23:09, Mingming Cao wrote:
> > On Tue, 2008-05-13 at 16:54 +0200, Jan Kara wrote:
> > > On Mon 12-05-08 17:39:43, Mingming Cao wrote:
> > > > On Mon, 2008-05-12 at 17:54 +0200, Jan Kara wrote:
> > > > Does this match what you are thinking? It certainly slow down the DIO
> > > > path, but the positive side is it doesn't disturb the other code path...
> > > > thanks for your feedback!
> > > > 
> > > > --------------------------------------------
> > > > 
> > > > An unexpected EIO error gets returned when writing to a file
> > > > using buffered writes and DIO writes at the same time.
> > > > 
> > > > We found there are a number of places where journal_try_to_free_buffers()
> > > > could race with journal_commit_transaction(), the later still
> > > > helds the reference to the buffers on the t_syncdata_list or t_locked_list
> > > > , while journal_try_to_free_buffers() tries to free them, which resulting an EIO
> > > > error returns back to the dio caller. 
> > > > 
> > > > The logic fix is to retry freeing if journal_try_to_free_buffers() to failed
> > > > to free those data buffers while journal_commit_transaction() is still
> > > > reference those buffers.
> > > > This is done via implement ext3 launder_page() callback, instead of inside
> > > > journal_try_to_free_buffers() itself, so that it doesn't affecting other code
> > > > path calling journal_try_to_free_buffers and only dio path get affected.
> > > > 
> > > > Signed-off-by: Mingming Cao <cmm@...ibm.com>
> > > > Index: linux-2.6.26-rc1/fs/ext3/inode.c
> > > > ===================================================================
> > > > --- linux-2.6.26-rc1.orig/fs/ext3/inode.c	2008-05-03 11:59:44.000000000 -0700
> > > > +++ linux-2.6.26-rc1/fs/ext3/inode.c	2008-05-12 12:41:27.000000000 -0700
> > > > @@ -1766,6 +1766,23 @@ static int ext3_journalled_set_page_dirt
> > > >  	return __set_page_dirty_nobuffers(page);
> > > >  }
> > > >  
> > > > +static int ext3_launder_page(struct page *page)
> > > > +{
> > > > +        int ret;
> > > > +	int retry = 5;
> > > > +
> > > > +	while (retry --) {
> > > > +		ret = ext3_releasepage(page, GFP_KERNEL);
> > > > +		if (ret == 1)
> > > > +			break;
> > > > +		else
> > > > +			schedule();
> > > > +	}
> > > > +
> > > > +        return ret;
> > > > +}
> > > > +
> > > > +
> > >   Yes, I meant something like this. We could be more clever and do:
> > > 
> > > 	head = bh = page_buffers(page);
> > > 	do {
> > > 		wait_on_buffer(bh);
> > > 		bh = bh->b_this_page;
> > > 	} while (bh != head);
> > > 	/*
> > > 	 * Now commit code should have been able to proceed and release
> > >          * those buffers
> > > 	 */
> > >         schedule();
> > > 
> > 
> > Bummer, we can't free buffers in ext3_launder_page() before calling
> > try_to_free_page, as later
> > invalidate_complete_page2()->try_to_free_page() expecting the page
> > buffers are still here, and will return EIO if it launder_page() has
> > already freed those buffers.:(
>   Are you sure? Because if bufferes are released in ext3_launder_page(),
> PagePrivate() has been set to 0 and we should directly fall through to
> releasing the page without ever calling try_to_release_page()... So I'd
> want to find out why PagePrivate is still set in
> invalidate_complete_page2().
> 

You are right. PagePrivate() is being set to 0 in drop_buffers(). 

The problem is do_launder_page() returns successfully if the page is not
dirty (our case), so ext3_launder_page() is not even get called. This
also explains why the log_wait_commit() approach doesn't work for me:(

Have to think other ways...could we pass some flag to
journal_try_to_free_buffers(), and ask journal_try_to_free_buffers()
wait for jbd commit to finish flushing the data, if the request is from
directo IO?


Mingming

--
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html