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 ?
Regards,
Ryusuke Konishi
--
To unsubscribe from this list: send the line "unsubscribe linux-nilfs" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
