Issue #3312 has been updated by tkusumi.
> Definitely not a bug. hammer2_chain_modify() must be called prior to any > intent to modify content, instead of assuming that the chain is in a > particular state due to some other procedure call (such as a > hammer2_chain_create()). I understand it's not a bug, but what I want to know is whether the second one is technically mandatory or not in this particular call sequence. >From what I understand it's not mandatory. It sounds to me you are saying it's for code clarity to locate hammer2_chain_modify() in the same place in the same function whenever chain data gets modified, regardless of whether some other function has implicitly done it right before, which of course makes sense. > It is very important that there be no confusion there, because modifying the > content of a chain's data with the chain in the wrong state can cause chaos. > Also, modifying the buffer cache without properly configuring the state of > the underlying buffer can cause chaos. But in this particular case I mentioned, not having the second one won't cause any chaos, which is why I think it's redundant. Having said that, I'm not saying the second one should be removed. ---------------------------------------- Submit #3312: hammer2: redundant chain modify after chain creation http://bugs.dragonflybsd.org/issues/3312#change-14285 * Author: tkusumi * Status: New * Priority: Normal * Target version: 6.4 * Start date: 2022-02-05 ---------------------------------------- It looks to me hammer2_chain_modify() calls right after hammer2_chain_create() in creat/mkdir/etc syscalls are all redundant. Take a look at hammer2_xop_inode_create_det() for example. A caller process always passes NULL chain to hammer2_chain_create(), so it always allocates a new chain and then calls hammer2_chain_modify() which only initializes a new buf for devvp. After successful hammer2_chain_create(), a caller explicitly calls hammer2_chain_modify() for the second time. The chain is already modified + has non-zero data_off, so it breads data for buf from above (which I believe contains garbage at this point). After that a caller copies ondisk inode to its chain data which is a pointer to somewhere in devvp's buf data. The flusher eventually recursively flushes chain data to devvp along with flushing vp's buf data (user data) to devvp. What I don't understand is why the second hammer2_chain_modify() is needed. The first one called from hammer2_chain_create() seems good enough in these cases. In fact I've had no issue with this diff to test above. https://leaf.dragonflybsd.org/~tkusumi/diff/0001-hammer2-omit-redundant-chain-modify-after-creation.patch -- You have received this notification because you have either subscribed to it, or are involved in it. To change your notification preferences, please click here: http://bugs.dragonflybsd.org/my/account
