DragonFlyBSD

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

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.