[DragonFlyBSD - Submit #3312] hammer2: redundant chain modify after chain creation

[bugtracker-admin at leaf.dragonflybsd.org]

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