Re: [PATCH] log replay should not overwrite newer ondisk inodes

[Lachlan McIlroy <lachlan@sgi.com>]

Timothy Shimmin wrote:
> Shailendra Tripathi wrote:
>> Hi,
>>      Can someone explain how not checking the flushiter can losse 
>> filesize updates.
>> Let me the take the case mentioned here in the fix statement:
>>
>> a. Clustered inode create -  flush iter - X( 0)
>> b. size update  --> flush iter --> Y
>>
>> X and Y will always hold as: X <= Y, that is, it is not possible to 
>> have X >Y (unless size update is non -transactional. As far as I know, 
>> size update is always transactional.)
>>
>> There are 2 cases here:
>> a) log of Y reached to the disk  --> 1) inode with flush iter was 
>> reached 2) inode didn't make.
>> b) log of Y didn't reach the disk --> flush_iter Y should have never 
>> reached disk
>>
>> In none of cases, I can see the possibility that size update can be 
>> lost becuase of replaying of the logs in the sequential order. If Log 
>> of Y didn't reach, does it not make sense to have the flush_iter and 
>> size correctly set to the last known transaction on the disk. To me, 
>> it appears unsafe to do as the inode state will not match the state of 
>> the last known transaction after recovery.
>>
>> Regards,
>> Shailendra
> 
> Dave answered this but yes this is a case where we are breaking
> the transaction model IMO. And my understanding is that we are doing
> this for performance reasons.
> One of Lachlan's proposals (IIRC) was to log the size change before the
> ondisk size change in xfs_aops.c/xfs_setfilesize()
> and thus follow the model but questions were raised about introducing
> performance overhead of log traffic in the write path.
> 

It would make life easier to do it that way - we wouldn't have to check
the flushiter field of the ondisk inode because we know we will end up
with the same thing by just replaying the log.  But since the addition
of the flushiter stuff pre-dates the NULL files changes there must be
another reason we need it.  Previously the size change would get logged
with an extent allocation transaction but extending a file within the
same last block would not send the new file size to the log.  I think
that may have been the reason for needing the flushiter stuff.  If we
log the file size change in xfs_setfilesize() we may not need the
flushiter stuff at all, hmmm maybe for timestamp updates?