Submit #3312
open
hammer2: redundant chain modify after chain creation
0%
Description
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
Updated by dillon almost 2 years ago
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()). 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.
What should happen here is that the newly allocated chain will usually have the HAMMER2_CHAIN_INITIAL flag set. e.g. line 3421 of hammer2_chain.c. In most cases. This will cause hammer2_chain_modify() to issue hammer2_io_new() instead of hammer2_io_bread() in hammer2_chain_modify() -> hammer2_chain_load_data() around line 1205. This will construct a dirty buffer with all zero's content instead of trying to read the block from disk.
There are numerous special cases here. The buffer cache and the hammer2_dio cache use 64KB blocks. If a hammer2_chain represents a smaller block, the bread must still be done because other portions of that block may be referenced by other parts of the filesystem. If a 64KB chain is being allocated, though, it should be able to avoid the bread.
There is a further optimization in the allocator when allocating small blocks out of an initially fully-free 64KB block to also avoid the bread operation. This catches the remaining cases (typically chains with smaller block sizes than the natural 64KB block size). That is around line 756 of hammer2_freemap.c. Here, if allocating any block size out of an initially fully-free 64KB area, hammer2_io_newnz() is called to create a dirty pre-zero'd backing for that in the DIO and buffer cache (which avoids having to do a bread()).
The underlying buffer cache buffer will remain dirty for some time, usually tens of seconds before being flushed, and thus be able to catch most bulk operations without forcing a bread().
-Matt
Updated by tkusumi almost 2 years ago
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.