xfs
[Top] [All Lists]

Re: update: Re: XFS 1.2 bug?

To: Steven Dake <sdake@xxxxxxxxxx>
Subject: Re: update: Re: XFS 1.2 bug?
From: Rusell Cattelan <cattelan@xxxxxxx>
Date: 26 Mar 2003 13:00:27 -0600
Cc: Russell Cattelan <cattelan@xxxxxxxxxxx>, linux-xfs@xxxxxxxxxxx
In-reply-to: <3E81E1F2.2040001@mvista.com>
Organization:
References: <3E809EB1.6000904@mvista.com> <1048639851.11129.44.camel@rose.americas.sgi.com> <3E80FEB5.90801@mvista.com> <3E8106CD.2050101@mvista.com> <3E8151E4.809@thebarn.com> <3E81E1F2.2040001@mvista.com>
Sender: linux-xfs-bounce@xxxxxxxxxxx
On Wed, 2003-03-26 at 11:22, Steven Dake wrote:
> Russell thanks for the responses and questions below.
> 
> Russell Cattelan wrote:
> 
> > Ok I've looked over the code.
> > write_full_page is modeled after block_write_full_page.
> 
> Is block_write_full_page something new in XFS CVS?  I don't see it in 1.2...
block_write_full_page is the generic write_full_page use by most file
systems.
eg
static int ext2_writepage(struct page *page)
{
        return block_write_full_page(page,ext2_get_block);
}

The check for writing beyond the end of file is apparently taken from
there.
        /* easy case */
        if (page->index < end_index)
                return __block_write_full_page(inode, page, get_block);

        /* things got complicated... */
        offset = inode->i_size & (PAGE_CACHE_SIZE-1);
        /* OK, are we completely out? */
        if (page->index >= end_index+1 || !offset) {
                UnlockPage(page);
                return -EIO;
        }


> 
> > The past the end of file is the same check used in that function.
> > Basically the offset var used to see if size of the file is part way
> > into a page so if the size of the file lands on a page boundry offset
> > will be 0.
I've been looking at the code and I think it's correct to error 
in the case where page->index == i_size (unit converted ofcourse)
That would be the page just past the end if file.

> 
> I understand that the code is doing that, but I was wondering why XFS 
> should care that write_full_page should fail if the file size is on a 
> page boundary.  If the file size is on a page boundary, shouldn't XFS be 
> able to write the page just fine?
> 
> >
> > As far as the unlock_page in the error case ... again based on 
> > block_write_full_page
> > it seems correct to unlock the page in case of an error.
> 
> If you unlock it here, you must not unlock it in 
> generic_file_write_nolock.  It can only be unlocked once...  So the code 
> should pick where it shall unlock.  Since it will be unlocked later in 
> generic_file_write_nolock, I don't understand the need to unlock it in 
> write_full_page at all.  The only case I can think of unlocking the 
> page, is when it is _not_ called from generic_file_write_nolock. 
>  Perhaps this is happening somewhere and that is the need for the 
> unlock, but if that is the case, generic_file_write_nolock must not 
> unlock the page a second time...  Tthe error code for write_full_page is 
> never passed up to generic_file_write_nolock, so 
> generic_file_write_nolock is never aware that it should _not_ unlock the 
> page because the I/O had a previous error
Yes I agree the unlock path does looks suspicious.
I found 5 spots in the kernel the calls writepage only one
of them filemap_fdatasync actually checks the error code and
it even that doesn't change if the page is unlocked or not.

So yes I think the unlock_page is in error in write_full_page.... and
in block_write_full_page for that matter.


Stick a BUG() in the check for past eof ... the root of the problem seem
to be somebody trying to write past eof.



<Prev in Thread] Current Thread [Next in Thread>