[BUG] reiser4: page lock recursion in reiser4_write_extent

[BUG] reiser4: page lock recursion in reiser4_write_extent

[Nate Diller]

This little code snippet seems to have a page_lock recursion, in
addition to overall looking particularly fragile to me.  It seems to
be handling the case where a page needs to be brought uptodate because
a partial page write is being done.  The page gets locked as many as 3
times, each checking PageUptodate, however the two failure cases here
go BUG() instead of returning an error.  I'm starting to think that
somehow the whole suspect branch just never gets taken, because
otherwise I would expect to see bug reports related to -EIO, -ENOMEM,
etc causing this to barf.

either way, it seems there's a lock recursion if another thread races
to bring @page uptodate while we're waiting on the first lock_page()
call.

---

                page = jnode_page(jnodes[i]);
                if (page_offset(page) < inode->i_size &&
                    !PageUptodate(page) && to_page != PAGE_CACHE_SIZE) {
                        /*
                         * the above is not optimal for partial write to last
                         * page of file when file size is not at boundary of
                         * page
                         */
takes the lock
                        lock_page(page);
raced with readpage?
                        if (!PageUptodate(page)) {
readpage drops lock
                                result = readpage_unix_file(NULL, page);
                                BUG_ON(result != 0);
-ENOMEM?
                                /* wait for read completion */
                                lock_page(page);
                                BUG_ON(!PageUptodate(page));
-EIO?
                                unlock_page(page);
                        } else
still have the lock here
                                result = 0;
                }

                BUG_ON(get_current_context()->trans->atom != NULL);
                fault_in_pages_readable(buf, to_page);
                BUG_ON(get_current_context()->trans->atom != NULL);

BOOM!!!
                lock_page(page);
                if (!PageUptodate(page) && to_page != PAGE_CACHE_SIZE) {

---

NATE

Re: [BUG] reiser4: page lock recursion in reiser4_write_extent

[Vladimir V. Saveliev]

Hello

On Wednesday 14 March 2007 08:40, Nate Diller wrote:
> This little code snippet seems to have a page_lock recursion, in
> addition to overall looking particularly fragile to me.  It seems to
> be handling the case where a page needs to be brought uptodate because
> a partial page write is being done.  The page gets locked as many as 3
> times, each checking PageUptodate, however the two failure cases here
> go BUG() instead of returning an error.  I'm starting to think that
> somehow the whole suspect branch just never gets taken, because
> otherwise I would expect to see bug reports related to -EIO, -ENOMEM,
> etc causing this to barf.
> 
> either way, it seems there's a lock recursion if another thread races
> to bring @page uptodate while we're waiting on the first lock_page()
> call.
> 
> ---
> 
>                 page = jnode_page(jnodes[i]);
>                 if (page_offset(page) < inode->i_size &&
>                     !PageUptodate(page) && to_page != PAGE_CACHE_SIZE) {
>                         /*
>                          * the above is not optimal for partial write to last
>                          * page of file when file size is not at boundary of
>                          * page
>                          */
> takes the lock
>                         lock_page(page);
> raced with readpage?
>                         if (!PageUptodate(page)) {
> readpage drops lock
>                                 result = readpage_unix_file(NULL, page);
>                                 BUG_ON(result != 0);
> -ENOMEM?
>                                 /* wait for read completion */
>                                 lock_page(page);
>                                 BUG_ON(!PageUptodate(page));
> -EIO?
>                                 unlock_page(page);
>                         } else
> still have the lock here
>                                 result = 0;
>                 }
> 
>                 BUG_ON(get_current_context()->trans->atom != NULL);
>                 fault_in_pages_readable(buf, to_page);
>                 BUG_ON(get_current_context()->trans->atom != NULL);
> 
> BOOM!!!
>                 lock_page(page);
>                 if (!PageUptodate(page) && to_page != PAGE_CACHE_SIZE) {

you are right, I will make a patch.

Re: [BUG] reiser4: page lock recursion in reiser4_write_extent

[Vladimir V. Saveliev]

Hello

On Wednesday 14 March 2007 08:40, Nate Diller wrote:
> This little code snippet seems to have a page_lock recursion, in
> addition to overall looking particularly fragile to me.  It seems to
> be handling the case where a page needs to be brought uptodate because
> a partial page write is being done.  The page gets locked as many as 3
> times, each checking PageUptodate, however the two failure cases here
> go BUG() instead of returning an error. 

yes, these are to be returned errors

> I'm starting to think that 
> somehow the whole suspect branch just never gets taken, because
> otherwise I would expect to see bug reports related to -EIO, -ENOMEM,
> etc causing this to barf.

> 
[snip]
> 
> BOOM!!!

2.6.21-rc3-mm2 has this bug fixed.

>                 lock_page(page);
>                 if (!PageUptodate(page) && to_page != PAGE_CACHE_SIZE) {
> 
> ---
> 
> NATE
> 
>