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

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

[-- Attachment #1: Type: text/plain, Size: 4182 bytes --]

On 2015-05-11 04:07, Ryusuke Konishi wrote:
> On Mon, 11 May 2015 03:23:23 +0900 (JST), Ryusuke Konishi wrote:
>> On Mon, 11 May 2015 03:15:12 +0900 (JST), Ryusuke Konishi wrote:
>>> On Sun,  3 May 2015 12:05:21 +0200, Andreas Rohner wrote:
>>>> +/**
>>>> + * nilfs_segctor_dec_nlive_blks_gc - dec. nlive_blks for blocks of GC-Inodes
>>>> + * @dat: dat inode
>>>> + * @segbuf: currtent segment buffer
>>>> + * @bh: current buffer head
>>>> + *
>>>> + * Description: nilfs_segctor_dec_nlive_blks_gc() is called if the inode to
>>>> + * which @bh belongs is a GC-Inode. In that case it is not necessary to
>>>> + * decrement the previous segment, because at the end of the GC process it
>>>> + * will be freed anyway. It is however necessary to check again if the blocks
>>>> + * are alive here, because the last check was in userspace without the proper
>>>> + * locking. Additionally the blocks protected by the protection period should
>>>> + * be considered reclaimable. It is assumed, that @bh->b_blocknr contains
>>>> + * a virtual block number, which is only true if @bh is part of a GC-Inode.
>>>> + */
>>>
>>>> +static void nilfs_segctor_dec_nlive_blks_gc(struct inode *dat,
>>>> +					    struct nilfs_segment_buffer *segbuf,
>>>> +					    struct buffer_head *bh) {
>>>> +	bool isreclaimable = buffer_nilfs_period_protected(bh) ||
>>>> +				nilfs_dat_is_live(dat, bh->b_blocknr) <= 0;
>>>> +
>>>> +	if (!buffer_nilfs_snapshot_protected(bh) && isreclaimable)
>>>> +		segbuf->sb_nlive_blks--;
>>>> +	if (buffer_nilfs_snapshot_protected(bh))
>>>> +		segbuf->sb_nsnapshot_blks++;
>>>> +}
>>>
>>> I have some comments on this function:
>>>
>>>  - The position of the brace "{" violates a CodingStyle rule of function.
>>>  - buffer_nilfs_snapshot_protected() is tested twice, but this can be
>>>    reduced as follows:
>>>
>>> 	if (buffer_nilfs_snapshot_protected(bh))
>>> 		segbuf->sb_nsnapshot_blks++;
>>> 	else if (isreclaimable)
>>> 		segbuf->sb_nlive_blks--;
>>>
>>>  - Additionally, I prefer "reclaimable" to "isreclaimable" since it's
>>>    simpler and still trivial.
>>>
>>>  - 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.
>>>
>>>    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:
>>>
>>> static void nilfs_segctor_dec_nlive_blks_gc(struct inode *dat,
>>> 					    struct nilfs_segment_buffer *segbuf,
>>> 					    struct buffer_head *bh)
>>> {
>>> 	if (buffer_nilfs_snapshot_protected(bh))
>>> 		segbuf->sb_nsnapshot_blks++;
>>
>>> 	else if (!buffer_nilfs_still_alive(bh) ||
>>> 		 nilfs_dat_is_live(dat, bh->b_blocknr) <= 0)
>>> 		segbuf->sb_nlive_blks--;
>>
>> This was wrong.  It should be:
>>
>> 	else if (!buffer_nilfs_still_alive(bh) &&
>> 		 nilfs_dat_is_live(dat, bh->b_blocknr) <= 0)
>> 		segbuf->sb_nlive_blks--;
> 
> Sorry for confusing you.  I read again the code, and now feel
> the previous one (the following) was rather correct.
> 
>>> 	if (buffer_nilfs_snapshot_protected(bh))
>>> 		segbuf->sb_nsnapshot_blks++;
>>> 	else if (!buffer_nilfs_still_alive(bh) ||
>>> 		 nilfs_dat_is_live(dat, bh->b_blocknr) <= 0)
>>> 		segbuf->sb_nlive_blks--;
> 
> Could you confirm which logic correctly implements the algorithm that
> you intended ?

This one is correct. We only have to call nilfs_dat_is_live() if the
block is alive. nilfs_dat_is_live() is intended to confirm that a block
is really live. If we know from userspace, that a block is
dead/reclaimable we do not have to check it again.

Regards,
Andreas Rohner

> Regards,
> Ryusuke Konishi
> 



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 801 bytes --]