On Mon, 31 Jul 2006 16:57:02 +0400
Evgeniy Dushistov <[email protected]> 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 <[email protected]>
>
>
> ---
>
>
> 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.
Andrew Morton wrote:
>On Mon, 31 Jul 2006 16:57:02 +0400
>Evgeniy Dushistov <[email protected]> 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 <[email protected]>
>>
>>
>>---
>>
>>
>>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
On Mon, Jul 31, 2006 at 11:02:51PM -0700, Andrew Morton wrote:
> On Mon, 31 Jul 2006 16:57:02 +0400
> Evgeniy Dushistov <[email protected]> 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
On Tue, 1 Aug 2006 11:30:43 +0400
Evgeniy Dushistov <[email protected]> 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 <[email protected]> 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?
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 <[email protected]>
---
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