From: Evgeniy Dushistov <dushistov@mail.ru>
To: Andrew Morton <akpm@osdl.org>
Cc: linux-kernel@vger.kernel.org, linux-fsdevel@vger.kernel.org
Subject: Re: [PATCH]: ufs: ufs_change_blocknr: skip truncated pages
Date: Tue, 1 Aug 2006 16:00:20 +0400 [thread overview]
Message-ID: <20060801120020.GA24263@rain> (raw)
In-Reply-To: <20060801003958.c628455f.akpm@osdl.org>
On Tue, Aug 01, 2006 at 12:39:58AM -0700, Andrew Morton wrote:
> I'm not sure that the `goto repeat' is needed if truncate got there first,
> really - if truncate took the page down then it's now outside i_size and
> shouldn't be coming back.
>
> If the page _can_ come back then this code is all rather problematic.
> Because this means that the page can come back (via an extending write())
> one nanosecond after ufs_get_locked_page() returns NULL. Won't the callers
> of ufs_get_locked_page() get confused by that?
ufs_get_locked_page is called twice in ufs code,
one time in ufs_truncate path(we allocated last block),
and another time when fragments are reallocated.
In ideal world in the second case on allocation/free block layer we
should not know that things like `truncate' exists, but now with
such crutch like ufs_get_locked_page we can (or should?) skip truncated pages.
Signed-off-by: Evgeniy Dushistov <dushistov@mail.ru>
---
Index: linux-2.6.18-rc2-mm1/fs/ufs/balloc.c
===================================================================
--- linux-2.6.18-rc2-mm1.orig/fs/ufs/balloc.c
+++ linux-2.6.18-rc2-mm1/fs/ufs/balloc.c
@@ -248,7 +248,7 @@ static void ufs_change_blocknr(struct in
if (likely(cur_index != index)) {
page = ufs_get_locked_page(mapping, index);
- if (IS_ERR(page))
+ if (!page || IS_ERR(page)) /* it was truncated or EIO */
continue;
} else
page = locked_page;
Index: linux-2.6.18-rc2-mm1/fs/ufs/util.c
===================================================================
--- linux-2.6.18-rc2-mm1.orig/fs/ufs/util.c
+++ linux-2.6.18-rc2-mm1/fs/ufs/util.c
@@ -251,7 +251,6 @@ struct page *ufs_get_locked_page(struct
{
struct page *page;
-try_again:
page = find_lock_page(mapping, index);
if (!page) {
page = read_cache_page(mapping, index,
@@ -271,7 +270,8 @@ try_again:
/* Truncate got there first */
unlock_page(page);
page_cache_release(page);
- goto try_again;
+ page = NULL;
+ goto out;
}
if (!PageUptodate(page) || PageError(page)) {
--
/Evgeniy
prev parent reply other threads:[~2006-08-01 11:52 UTC|newest]
Thread overview: 6+ messages / expand[flat|nested] mbox.gz Atom feed top
2006-07-31 12:57 [PATCH]: ufs: ufs_get_locked_patch race fix Evgeniy Dushistov
2006-08-01 6:02 ` Andrew Morton
2006-08-01 7:18 ` Nick Piggin
2006-08-01 7:30 ` Evgeniy Dushistov
2006-08-01 7:39 ` Andrew Morton
2006-08-01 12:00 ` Evgeniy Dushistov [this message]
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=20060801120020.GA24263@rain \
--to=dushistov@mail.ru \
--cc=akpm@osdl.org \
--cc=linux-fsdevel@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
/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.