* [PATCH]: ufs: ufs_get_locked_patch race fix
@ 2006-07-31 12:57 Evgeniy Dushistov
2006-08-01 6:02 ` Andrew Morton
0 siblings, 1 reply; 6+ messages in thread
From: Evgeniy Dushistov @ 2006-07-31 12:57 UTC (permalink / raw)
To: linux-kernel, linux-fsdevel; +Cc: Andrew Morton
As discussed earlier:
http://lkml.org/lkml/2006/6/28/136
this patch fixes such issue:
`ufs_get_locked_page' takes page from cache
after that `vmtruncate' takes page and deletes it from cache
`ufs_get_locked_page' locks page, and reports about EIO error.
Also because of find_lock_page always return valid page or NULL,
we have no need check it if page not NULL.
Signed-off-by: Evgeniy Dushistov <dushistov@mail.ru>
---
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
@@ -257,6 +257,7 @@ try_again:
page = read_cache_page(mapping, index,
(filler_t*)mapping->a_ops->readpage,
NULL);
+
if (IS_ERR(page)) {
printk(KERN_ERR "ufs_change_blocknr: "
"read_cache_page error: ino %lu, index: %lu\n",
@@ -266,6 +267,13 @@ try_again:
lock_page(page);
+ if (unlikely(page->mapping != mapping ||
+ page->index != index)) {
+ unlock_page(page);
+ page_cache_release(page);
+ goto try_again;
+ }
+
if (!PageUptodate(page) || PageError(page)) {
unlock_page(page);
page_cache_release(page);
@@ -275,15 +283,8 @@ try_again:
mapping->host->i_ino, index);
page = ERR_PTR(-EIO);
- goto out;
}
}
-
- if (unlikely(!page->mapping || !page_has_buffers(page))) {
- unlock_page(page);
- page_cache_release(page);
- goto try_again;/*we really need these buffers*/
- }
out:
return page;
}
--
/Evgeniy
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH]: ufs: ufs_get_locked_patch race fix
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
0 siblings, 2 replies; 6+ messages in thread
From: Andrew Morton @ 2006-08-01 6:02 UTC (permalink / raw)
To: Evgeniy Dushistov; +Cc: linux-kernel, linux-fsdevel
On Mon, 31 Jul 2006 16:57:02 +0400
Evgeniy Dushistov <dushistov@mail.ru> wrote:
> As discussed earlier:
> http://lkml.org/lkml/2006/6/28/136
> this patch fixes such issue:
> `ufs_get_locked_page' takes page from cache
> after that `vmtruncate' takes page and deletes it from cache
> `ufs_get_locked_page' locks page, and reports about EIO error.
>
> Also because of find_lock_page always return valid page or NULL,
> we have no need check it if page not NULL.
>
> Signed-off-by: Evgeniy Dushistov <dushistov@mail.ru>
>
>
> ---
>
>
> 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
> @@ -257,6 +257,7 @@ try_again:
> page = read_cache_page(mapping, index,
> (filler_t*)mapping->a_ops->readpage,
> NULL);
> +
> if (IS_ERR(page)) {
> printk(KERN_ERR "ufs_change_blocknr: "
> "read_cache_page error: ino %lu, index: %lu\n",
> @@ -266,6 +267,13 @@ try_again:
>
> lock_page(page);
>
> + if (unlikely(page->mapping != mapping ||
> + page->index != index)) {
> + unlock_page(page);
> + page_cache_release(page);
> + goto try_again;
> + }
> +
> if (!PageUptodate(page) || PageError(page)) {
> unlock_page(page);
> page_cache_release(page);
Looks good to me.
Is there any need to be checking ->index? Normally we simply use the
sequence:
lock_page(page);
if (page->mapping == NULL)
/* truncate got there first */
to handle this case.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH]: ufs: ufs_get_locked_patch race fix
2006-08-01 6:02 ` Andrew Morton
@ 2006-08-01 7:18 ` Nick Piggin
2006-08-01 7:30 ` Evgeniy Dushistov
1 sibling, 0 replies; 6+ messages in thread
From: Nick Piggin @ 2006-08-01 7:18 UTC (permalink / raw)
To: Andrew Morton; +Cc: Evgeniy Dushistov, linux-kernel, linux-fsdevel
Andrew Morton wrote:
>On Mon, 31 Jul 2006 16:57:02 +0400
>Evgeniy Dushistov <dushistov@mail.ru> wrote:
>
>
>>As discussed earlier:
>>http://lkml.org/lkml/2006/6/28/136
>>this patch fixes such issue:
>>`ufs_get_locked_page' takes page from cache
>>after that `vmtruncate' takes page and deletes it from cache
>>`ufs_get_locked_page' locks page, and reports about EIO error.
>>
>>Also because of find_lock_page always return valid page or NULL,
>>we have no need check it if page not NULL.
>>
>>Signed-off-by: Evgeniy Dushistov <dushistov@mail.ru>
>>
>>
>>---
>>
>>
>>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
>>@@ -257,6 +257,7 @@ try_again:
>> page = read_cache_page(mapping, index,
>> (filler_t*)mapping->a_ops->readpage,
>> NULL);
>>+
>> if (IS_ERR(page)) {
>> printk(KERN_ERR "ufs_change_blocknr: "
>> "read_cache_page error: ino %lu, index: %lu\n",
>>@@ -266,6 +267,13 @@ try_again:
>>
>> lock_page(page);
>>
>>+ if (unlikely(page->mapping != mapping ||
>>+ page->index != index)) {
>>+ unlock_page(page);
>>+ page_cache_release(page);
>>+ goto try_again;
>>+ }
>>+
>> if (!PageUptodate(page) || PageError(page)) {
>> unlock_page(page);
>> page_cache_release(page);
>>
>
>Looks good to me.
>
>Is there any need to be checking ->index? Normally we simply use the
>sequence:
>
> lock_page(page);
> if (page->mapping == NULL)
> /* truncate got there first */
>
>to handle this case.
>
If you already have a reference on the page, I believe no. We have to
be a bit careful about it in the core vm now that splice can move
pages between different parts of the same mapping, but any code outside
mm/ shouldn't need to worry (exactly because they should have a ref
before doing the lock_page).
--
Send instant messages to your online friends http://au.messenger.yahoo.com
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH]: ufs: ufs_get_locked_patch race fix
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
1 sibling, 1 reply; 6+ messages in thread
From: Evgeniy Dushistov @ 2006-08-01 7:30 UTC (permalink / raw)
To: Andrew Morton; +Cc: linux-kernel, linux-fsdevel
On Mon, Jul 31, 2006 at 11:02:51PM -0700, Andrew Morton wrote:
> On Mon, 31 Jul 2006 16:57:02 +0400
> Evgeniy Dushistov <dushistov@mail.ru> wrote:
>
> Looks good to me.
>
> Is there any need to be checking ->index? Normally we simply use the
> sequence:
>
> lock_page(page);
> if (page->mapping == NULL)
> /* truncate got there first */
>
> to handle this case.
Yes, I made it in analogy with `find_lock_page' and missed fact
that if we increment usage counter of page, we have no need to check
page->index.
Need another patch?
--
/Evgeniy
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH]: ufs: ufs_get_locked_patch race fix
2006-08-01 7:30 ` Evgeniy Dushistov
@ 2006-08-01 7:39 ` Andrew Morton
2006-08-01 12:00 ` [PATCH]: ufs: ufs_change_blocknr: skip truncated pages Evgeniy Dushistov
0 siblings, 1 reply; 6+ messages in thread
From: Andrew Morton @ 2006-08-01 7:39 UTC (permalink / raw)
To: Evgeniy Dushistov; +Cc: linux-kernel, linux-fsdevel
On Tue, 1 Aug 2006 11:30:43 +0400
Evgeniy Dushistov <dushistov@mail.ru> wrote:
> On Mon, Jul 31, 2006 at 11:02:51PM -0700, Andrew Morton wrote:
> > On Mon, 31 Jul 2006 16:57:02 +0400
> > Evgeniy Dushistov <dushistov@mail.ru> wrote:
> >
> > Looks good to me.
> >
> > Is there any need to be checking ->index? Normally we simply use the
> > sequence:
> >
> > lock_page(page);
> > if (page->mapping == NULL)
> > /* truncate got there first */
> >
> > to handle this case.
>
> Yes, I made it in analogy with `find_lock_page' and missed fact
> that if we increment usage counter of page, we have no need to check
> page->index.
OK. find_lock_page() has the splice stuff in it.
> Need another patch?
Is OK, I updated it.
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?
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH]: ufs: ufs_change_blocknr: skip truncated pages
2006-08-01 7:39 ` Andrew Morton
@ 2006-08-01 12:00 ` Evgeniy Dushistov
0 siblings, 0 replies; 6+ messages in thread
From: Evgeniy Dushistov @ 2006-08-01 12:00 UTC (permalink / raw)
To: Andrew Morton; +Cc: linux-kernel, linux-fsdevel
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
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2006-08-01 11:52 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
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 ` [PATCH]: ufs: ufs_change_blocknr: skip truncated pages Evgeniy Dushistov
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).