Re: [RFCv2 2/3] iomap: Change uptodate variable name to state

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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.



[Index of Archives]     [Linux Ext4 Filesystem]     [Union Filesystem]     [Filesystem Testing]     [Ceph Users]     [Ecryptfs]     [NTFS 3]     [AutoFS]     [Kernel Newbies]     [Share Photos]     [Security]     [Netfilter]     [Bugtraq]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux Cachefs]     [Reiser Filesystem]     [Linux RAID]     [NTFS 3]     [Samba]     [Device Mapper]     [CEPH Development]

  Powered by Linux