From mboxrd@z Thu Jan  1 00:00:00 1970
From: "Martin Jambor" <jambormartin@gmail.com>
Subject: Re: Relocking page in writepage
Date: Sun, 26 Feb 2006 16:32:40 +0100
Message-ID: <9615ac9b0602260732g5cf7f7dbs36f0c0f75e536c90@mail.gmail.com>
References: <9615ac9b0602150742v5b0911e2t1ee601716f4ed5dd@mail.gmail.com>
	 <20060225025053.2b25a91d.akpm@osdl.org>
Mime-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: 7BIT
Return-path: <linux-fsdevel-owner@vger.kernel.org>
Received: from xproxy.gmail.com ([66.249.82.206]:13348 "EHLO xproxy.gmail.com")
	by vger.kernel.org with ESMTP id S1751231AbWBZPcl convert rfc822-to-8bit
	(ORCPT <rfc822;linux-fsdevel@vger.kernel.org>);
	Sun, 26 Feb 2006 10:32:41 -0500
Received: by xproxy.gmail.com with SMTP id i26so457473wxd
        for <linux-fsdevel@vger.kernel.org>; Sun, 26 Feb 2006 07:32:40 -0800 (PST)
To: "Linux FS Development List" <linux-fsdevel@vger.kernel.org>
In-Reply-To: <20060225025053.2b25a91d.akpm@osdl.org>
Content-Disposition: inline
Sender: linux-fsdevel-owner@vger.kernel.org
List-Id: linux-fsdevel.vger.kernel.org

On 2/25/06, Andrew Morton <akpm@osdl.org> wrote:
> I can't immediately think of a problem with that.
>
> I guess it would be nicer to do:
>
>         lock_page();
>         if (page->mapping != mapping) {
>                 unlock_page();
>                 page_cache_release();
>
> so the page get freed immediately if the race happened, rather than having
> it drift down the LRU.
>
> umm, actually you do need to handle the case where someone came in and
> redirtied the page and potentially stated writeback against it.
>
>         lock_page();
>         wait_on_page_writeback();
>         if (page->mapping != mapping) {
>                 unlock_page();
>                 page_cache_release();
>                 return;
>         }
>
>         Now, we don't know whether to write the page.  Someone else might have
>         redirtied it and written it while w dropped the lock.
>
> So you have to go off and write it, occasionally unnecessarily.
>
> umm, no.  If you leave PageWriteback() set throughout, nobody will try to
> restart a write.

Well, I set writeback much later, only after I know there wouldn't be
any errors.
However, I was thinking if the following alternative solution would be ok:

set_page_writeback(page);
unlock_page(page)

/* do something that might deadlock */

lock_page(page);

if (error) {        /* -EIO, WRITEPAGE_ACTIVATE etc... */
        test_clear_page_writeback(page);
	wake_up_page(page, PG_writeback);
        return error;
}
/* continue as usual */

The problem with it is that test_clear_page_writeback() and
wake_up_page() are not exported for modules (which is a slight problem
at this stage of our project). On the other hand a page under
writeback shouldn't be stripped off its mapping or rewritten etc.
which simplifies things a lot.

Thanks for the input and any further comments, meanwhile I'll play
around with it and see what I can get away with.

Martin