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@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
