Re: [PATCH v2 8/9] nilfs2: correct live block tracking for GC protection period

[Andreas Rohner <andreas.rohner-hi6Y0CQ0nG0@public.gmane.org>]

On 2015-05-12 16:31, Ryusuke Konishi wrote:
> On Mon, 11 May 2015 15:00:44 +0200, Andreas Rohner wrote:
>> On 2015-05-10 20:15, Ryusuke Konishi wrote:
>>> On Sun,  3 May 2015 12:05:21 +0200, Andreas Rohner wrote:
>>>  - The logic of isreclaimable is counterintuitive.  
>>>
>>>> +	bool isreclaimable = buffer_nilfs_period_protected(bh) ||
>>>> +				nilfs_dat_is_live(dat, bh->b_blocknr) <= 0;
>>>
>>>    It looks like buffer_nilfs_period_protected(bh) here implies that
>>>    the block is deleted.  But it's independent from the buffer is
>>>    protected by protection_period or not.
>>
>> It is not independent. buffer_nilfs_period_protected() is only set for
>> deleted/reclaimable blocks that have to be copied because of the
>> protection period. So if the flag is set, then the block is always
>> reclaimable.
> 
> That is why it's confusing.
> Recall that we do not reclaim both deleted blocks and alive blocks 
> if they are protected by protection_period.
> 
> This naming and logic, by contries, treats period_protected blocks are
> "reclaimable".  That's the principal cause of this confusion.
> 
>>
>>>    Why not just adding "still alive" or "deleted" flag and its
>>>    corresponding vdesc flag instead of adding the period protected
>>>    flag ?
>>>
>>>    If we add the "still alive" flag, which means that the block is
>>>    not yet deleted from the latest checkpoint, then this function
>>>    can be simplified as follows:
>>
>> I think buffer_nilfs_period_protected(bh) is a better name.
>>
>> It does not mark all blocks within the protection period. Live blocks
>> within the protection period do not have this flag set.
> 
> I know.  That is my discussion point.
> 
>> It marks exactly
>> those blocks that are dead and reclaimable but protected from being
>> discarded by the protection period.
>> The protection period is key.
>> Without the protection period those blocks would not have been copied.
>> That is the exact meaning of the flag, and I think the name fits quite well.
> 
> I say that the flagging is confusing.  It's not simple (nor clear) at
> all.
> 
> Copied blocks by GC can have some properties.  For instance,
> 
>  1. snapshot protected  (guarded by one or more snapshots)
>  2. period protected  (guarded by protection_period)
>  3. deleted  (protected but isn't surviving on the latest checkpoint)
> 
> Among these, the property 2 does not relate to your live block
> counting algorithm; your live block counting algorithm uses
> property 1 and property 3 in reality.
> 
> If the algorithm counts the buffer marked "period protected" as alive
> and prevents nlive_blks count from being decremented, then I agree it
> "uses" the property 2.  But this patch doesn't
> 
> You are giving the property 2 to the period protection flag, and
> making it imply the property 3, which is intrinsically independent of
> the property 2.  And, are using the implication (property 3) instead
> of the title property 2.
> 
> Please think about it once again.

Thank you for the detailed explanation of your reasoning. I can see now
that it could be confusing. Maybe I have just gotten used to calling it
"period_protected". I will change the name to "deleted" in the next
version of the patch.

Regards,
Andreas Rohner

> Regards,
> Ryusuke Konishi
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-nilfs" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html