Re: [RFC] exofs: New truncate sequence

[Nick Piggin <npiggin@suse.de>]

On Mon, May 31, 2010 at 05:13:34PM +0300, Boaz Harrosh wrote:
> On 05/31/2010 04:44 PM, Nick Piggin wrote:
> > On Mon, May 31, 2010 at 03:30:02PM +0300, Boaz Harrosh wrote:
> >> ---
> >>  fs/exofs/exofs.h |    1 -
> >>  fs/exofs/file.c  |    1 -
> >>  fs/exofs/inode.c |  115 +++++++++++++++++++++++++++---------------------------
> > 
> > Can you rip out all the rest of the buffer_head stuff too?
> > 
> 
> I hope I don't have any left, that was the last, have I missed
> something?

exofs_invalidatepage, exofs_releasepage, includes of buffer_head.h.
No point to any of that if you never actually map the buffers or
use them for tracking state yourself.


> >> @@ -750,6 +760,10 @@ static int exofs_write_end(struct file *file, struct address_space *mapping,
> >>  	int ret;
> >>  
> >>  	ret = simple_write_end(file, mapping,pos, len, copied, page, fsdata);
> >> +	if (unlikely(ret && pos + len > inode->i_size))
> >> +		truncate_pagecache(inode, pos + len, inode->i_size);
> >> +
> > 
> > So there is no need to do any oi_truncate? Even if _readpage in
> > write_begin has set up some blocks?
> > 
> 
> No blocks are setup. Exofs does not have any blocks.

Sure, I was just using blocks as a placeholder for whatever you call
it.


> If the write
> failed then the object on OSD has the last written offset as objects
> size. If error handling was done right residual is subtracted from IO
> size and should be reflected here.
> 
> If IO was never attempted then object size did not grow and we revert
> in memory i_size here. (size is keept in two places and is checked
> for consistency in fsck)

So long as the _readpage has not altered the object state at all,
then this looks fine to me.

 
> TODO:
> 	eject short writes and see if this works correctly.
> 
> > 
> >> +	/* TODO: once simple_write_end marks inode dirty remove */
> >>  	if (i_size != inode->i_size)
> >>  		mark_inode_dirty(inode);
> >>  	return ret;
> > 
> > Hmm, I suppose simple_write_end probably should mark the inode dirty?
> > 
> 
> I think Christoph has a patch for that.
> 
> > 
> >> @@ -1335,28 +1339,25 @@ void exofs_delete_inode(struct inode *inode)
> >>  
> >>  	truncate_inode_pages(&inode->i_data, 0);
> >>  
> >> +	/* TODO: should do better here */
> >>  	if (is_bad_inode(inode))
> >>  		goto no_delete;
> >>  
> >> -	mark_inode_dirty(inode);
> >> -	exofs_update_inode(inode, inode_needs_sync(inode));
> >> -
> >>  	inode->i_size = 0;
> >> -	if (inode->i_blocks)
> >> -		exofs_truncate(inode);
> > 
> > This guy has gone missing -- I assume exofs_sbi_remove is a more
> > efficient way to do this anyway?
> > 
> 
> You see this is where exofs is different a file is an object_no on
> multiple OSD devices. The inode is kept as an attribute of the
> object. (data as object's data) so a exofs_sbi_remove will just
> obliterate any association to the object. It was historically
> called because exofs_truncate used to do what truncate_inode_pages
> does today. (And some other in memory book keeping.) But with
> your help all this was cleaned up.

OK, I was thinking the underlying object itself needs to be trimmed
to match i_size similarly to just a block based filesystem? Like
exofs_oi_truncate appears to.

 
> Do you see any operation I missed that might need cleaning from the
> generic VFS inode, that might now leak. As far as storage is concerned
> I'm covered.
> 
> [I ran git clone linux; rm -rf linux; 100 times in a loop and the OSD
>  storage stayed constant size. So I presume there is no storage leak.
>  OSD is good in this respect]

I can't see anything off hand. Was just flagging points where 
vmtruncate or truncate had been called and is not now. If you
have all those covered, then you should be OK.

 
> >> -
> >>  	clear_inode(inode);
> >>  
> >> -	ret = exofs_get_io_state(&sbi->layout, &ios);
> >> -	if (unlikely(ret)) {
> >> -		EXOFS_ERR("%s: exofs_get_io_state failed\n", __func__);
> >> -		return;
> >> -	}
> >> -
> >>  	/* if we are deleting an obj that hasn't been created yet, wait */
> >>  	if (!obj_created(oi)) {
> >>  		BUG_ON(!obj_2bcreated(oi));
> >>  		wait_event(oi->i_wq, obj_created(oi));
> >> +		/* ignore the error attempt a remove anyway */
> >> +	}
> >> +
> >> +	/* Now Remove the OSD objects */
> >> +	ret = exofs_get_io_state(&sbi->layout, &ios);
> >> +	if (unlikely(ret)) {
> >> +		EXOFS_ERR("%s: exofs_get_io_state failed\n", __func__);
> >> +		return;
> >>  	}
> >>  
> >>  	ios->obj.id = exofs_oi_objno(oi);
> >> -- 
> >> 1.6.6.1
> 
> Thanks for lookin. And thanks for making this patch possible. I wanted
> this cleaned, long ago, but it was only made easy and simple after your
> changes.

That's OK, thanks for helping with it.