On Tue, Jan 31, 2023 at 08:56:23AM +1100, Dave Chinner wrote: > > + spinlock_t state_lock; > > + unsigned long state[]; > > I don't realy like this change, nor the followup in the next patch > that puts two different state bits somewhere inside a single bitmap. I think that's due to the open-coding of accesses to those bits. Which was why I questioned the wisdom of sending out this patchset without having introduced the accessors. > This is the reason I don't like it - we lose the self-documenting > aspect of the code. bitmap_fill(iop->uptodate, nr_blocks) is > obviously correct, the new version isn't because "state" has no > obvious meaning, and it only gets worse in the next patch where > state is changed to have a magic "2 * nr_blocks" length and multiple > state bits per block. Completely agreed. > Having an obvious setup where there are two bitmaps, one for dirty > and one for uptodate, and the same bit in each bitmap corresponds to > the state for that sub-block region, it is easy to see that the code > is operating on the correct bit, to look at the bitmap and see what > bits are set, to compare uptodate and dirty bitmaps side by side, > etc. It's a much easier setup to read, code correctly, analyse and > debug than putting multiple state bits in the same bitmap array at > different indexes. > > If you are trying to keep this down to a single allocation by using > a single bitmap of undefined length, then change the declaration and > the structure size calculation away from using array notation and > instead just use pointers to the individual bitmap regions within > the allocated region. Hard to stomach that solution when the bitmap is usually 2 bytes long (in Ritesh's case). Let's see a version of this patchset with accessors before rendering judgement. Although to my mind, we still want a solution that scales beyond a bitmap. But a proper set of accessors will abstract that away.
