Re: [PATCH 0/6] first step toward the new truncate sequence

[Boaz Harrosh <bharrosh@panasas.com>]

On 05/31/2010 04:15 PM, Nick Piggin wrote:
> On Mon, May 31, 2010 at 01:17:35PM +0300, Boaz Harrosh wrote:
>> On 05/31/2010 12:50 PM, Boaz Harrosh wrote:
>>> On 05/30/2010 11:49 PM, Christoph Hellwig wrote:
>>>> This series is something that I see as the first major step towards
>>>> a broad switch to the new truncate sequence.  The patches get rid
>>>> of the _newtrunc variant of blockdev_direct_IO & friends and
>>>> *_write_begin, and clean up some bits in that area that make the
>>>> switch easier.  After this we have all vmtruncate instances except
>>>> for inode_setattr in filesystem code.  A second series to deal
>>>> with ->setattr will follow and after that we can easily switch
>>>> over one filesystem after another.
>>>>
>>>> I think this is still 2.6.34 material as it will make the fs
>>>> switches a lot easier and avoid introducing the _newtrunc variants
>>>> for one kernel release.
>>>> --
>>>> To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
>>>> the body of a message to majordomo@vger.kernel.org
>>>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>>
>>>
>>> Christoph, Nick hi.
>>>
>>> I'm attempting a truncate conversion for exofs, will need a review.
>>>
>>> I'm basing the first attempt on Al's next tree that has Nicks last
>>> patches. Will rebase on these when they get into that tree. Please
>>> advise on the best tree to use?
>>> (BTW did you mean 2.6.35 above, I guess)
>>>
>>> So one minor thing ext2_setsize can be static: 
> 
> Thanks.
> 
> 
>>> (It used to be used by the struct inode_operations)
>>>
>>
>> One more thing. ext2_setsize is only used by ext2_setattr do we still
>> need this code? (form ext2_setsize) half of it is done in ext2_setattr
>> already)
>>
>> 	inode->i_mtime = inode->i_ctime = CURRENT_TIME_SEC;
>> 	if (inode_needs_sync(inode)) {
>> 		sync_mapping_buffers(inode->i_mapping);
>> 		ext2_sync_inode (inode);
>> 	} else {
>> 		mark_inode_dirty(inode);
>> 	}
>>
>> And generic_setattr does some more of that, No?
> 
> Not sure. Some callers appear not to set ATTR_CTIME/ATTR_MTIME when
> making ATTR_SIZE changes. And I don't know the history of the required
> sync semantics here either.
> 
> I think these would be good questions to raise with the ext?
> maintainers, though. ext3/4 have similar code.
> 

OK, I've rechecked and and no one is doing:
	inode->i_mtime = inode->i_ctime = CURRENT_TIME_SEC;
So this one is needed as is.

But the:
 	if (inode_needs_sync(inode)) {
 		sync_mapping_buffers(inode->i_mapping);
 		ext2_sync_inode (inode);
 	} else {
 		mark_inode_dirty(inode);
 	}

Is just stupid given that mark_inode_dirty(inode); is
again done in ext2_setattr() right after the call to
_setsize. Perhaps if needed, then move it into _setattr

(Please also have a look at my exofs: truncate patch I would love your input)
Boaz