Re: Review: ensure EOF writes into existing extents update filesize

[David Chinner <dgc@sgi.com>]

On Tue, May 22, 2007 at 02:10:14PM +1000, Nathan Scott wrote:
> On Tue, 2007-05-22 at 11:03 +1000, David Chinner wrote:
> > On Tue, May 22, 2007 at 11:02:59AM +1000, Nathan Scott wrote:
> > > On Tue, 2007-05-22 at 10:44 +1000, David Chinner wrote:
> > > > On Mon, May 21, 2007 at 04:34:26PM +1000, Timothy Shimmin wrote:
> > > > I guess I need to ping Christoph and Nathan on this one....
> > > 
> > > Could you resend the patch to me please?  I lost the previous copy
> > > while ruthlessly ploughing through my mail backlog.  ;)
> > 
> > Below.
> 
> Looks pretty good to me - xfs_convert_page has been overlooked, I
> think - attached patch fixes that.

I thought about that, then tried to trip the problem and was not
successful. AFAICT, if we have multiple pages dirty and in the same
state (i.e. pwrite 0 32769 to dirty 3 pages, then sync, then pwrite
0 33000 to dirty and extend) we should hit the case where we cluster
the dirty pages and call xfs_convert_page() on the last two pages.

In that case, the entire range should be mapped via the one iomap
and so the last buffer (the one we extended) should be added to an
ioend with a type of 0 (IOMAP_READ) and hence we should see the bug. 
With the patch I posted, I can't get this to show the problem. It
should, but it doesn't....

I'll make the change anyway to be safe, but I'm still perplexed
as to why it doesn't seem necessary....

Ah - there it is - xfs_is_delayed_page():

    699                         if (buffer_unwritten(bh))
    700                                 acceptable = (type == IOMAP_UNWRITTEN);
    701                         else if (buffer_delay(bh))
    702                                 acceptable = (type == IOMAP_DELAY);
    703                         else if (buffer_dirty(bh) && buffer_mapped(bh))
    704     >>>>>>>>>>>                 acceptable = (type == 0);
    705                         else
    706                                 break;

The ioend we started with now has type = IOMAP_NEW = 0x40 which 
means xfs_convert_page() aborts clustering this case immediately.
IOWs, we are never getting to this xfs_convert_page() case and
we are only passing through xfs_page_state_convert for mapped
pages.

> You also initialise iomap_valid
> twice inside xfs_page_state_convert now ... I reverted that to just
> the once.

I'll fold these changes in and fixup xfs_is_delayed_page() to look
for type == IOMAP_NEW and send out a new patch. Thanks, Nathan.

Cheers,

Dave.
-- 
Dave Chinner
Principal Engineer
SGI Australian Software Group