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:
signature.asc
Description: OpenPGP digital signature
