* [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 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.