| Message ID | bf30b7bfb03ef368e6e744b3c63af3dbfa11304d.1675093524.git.ritesh.list@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | iomap: Add support for subpage dirty state tracking to improve write performance | expand |
On Mon, Jan 30, 2023 at 09:44:12PM +0530, Ritesh Harjani (IBM) wrote: > This patch just changes the struct iomap_page uptodate & uptodate_lock > member names to state and state_lock to better reflect their purpose for > the upcoming patch. > > Signed-off-by: Ritesh Harjani (IBM) <ritesh.list@gmail.com> > --- > fs/iomap/buffered-io.c | 30 +++++++++++++++--------------- > 1 file changed, 15 insertions(+), 15 deletions(-) > > diff --git a/fs/iomap/buffered-io.c b/fs/iomap/buffered-io.c > index e9c85fcf7a1f..faee2852db8f 100644 > --- a/fs/iomap/buffered-io.c > +++ b/fs/iomap/buffered-io.c > @@ -25,13 +25,13 @@ > > /* > * Structure allocated for each folio when block size < folio size > - * to track sub-folio uptodate status and I/O completions. > + * to track sub-folio uptodate state and I/O completions. > */ > struct iomap_page { > atomic_t read_bytes_pending; > atomic_t write_bytes_pending; > - spinlock_t uptodate_lock; > - unsigned long uptodate[]; > + 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. > }; > > static inline struct iomap_page *to_iomap_page(struct folio *folio) > @@ -58,12 +58,12 @@ iomap_page_create(struct inode *inode, struct folio *folio, unsigned int flags) > else > gfp = GFP_NOFS | __GFP_NOFAIL; > > - iop = kzalloc(struct_size(iop, uptodate, BITS_TO_LONGS(nr_blocks)), > + iop = kzalloc(struct_size(iop, state, BITS_TO_LONGS(nr_blocks)), > gfp); > if (iop) { > - spin_lock_init(&iop->uptodate_lock); > + spin_lock_init(&iop->state_lock); > if (folio_test_uptodate(folio)) > - bitmap_fill(iop->uptodate, nr_blocks); > + bitmap_fill(iop->state, nr_blocks); 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. 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. Cheers, Dave.
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.
On Mon, Jan 30, 2023 at 10:24:59PM +0000, Matthew Wilcox wrote: > > 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. Yes. I think what we need is proper helpers that are self-documenting for every bitmap update as I already suggsted last round. That keeps the efficient allocation, and keeps all the users self-documenting. It just adds a bit of boilerplate for all these helpers, but that should be worth having the clarity and performance benefits.
On 23/01/31 07:07AM, Christoph Hellwig wrote: > On Mon, Jan 30, 2023 at 10:24:59PM +0000, Matthew Wilcox wrote: > > > 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. > > Yes. I think what we need is proper helpers that are self-documenting > for every bitmap update as I already suggsted last round. That keeps > the efficient allocation, and keeps all the users self-documenting. > It just adds a bit of boilerplate for all these helpers, but that > should be worth having the clarity and performance benefits. Are these accessor apis looking good to be part of fs/iomap/buffered-io.c The rest of the changes will then be just using this to set/clear/test the uptodate/dirty bits of iop->state bitmap. I think, after these apis, there shouldn't be any place where we need to directly manipulate iop->state bitmap. These APIs are all what I think are required for current changeset. Please let me know your thoughts/suggestions on these. If it looks good, then I can fold these in, in different patches which implements uptodate/dirty functionality and send rfcv3 along with other review comments addressed. /* * Accessor functions for setting/clearing/checking uptodate and * dirty bits in iop->state bitmap. * nrblocks is i_blocks_per_folio() which is passed in every * function as the last argument for API consistency. */ static inline void iop_set_range_uptodate(struct iomap_page *iop, unsigned int start, unsigned int len, unsigned int nrblocks) { bitmap_set(iop->state, start, len); } static inline void iop_clear_range_uptodate(struct iomap_page *iop, unsigned int start, unsigned int len, unsigned int nrblocks) { bitmap_clear(iop->state, start, len); } static inline bool iop_test_uptodate(struct iomap_page *iop, unsigned int pos, unsigned int nrblocks) { return test_bit(pos, iop->state); } static inline bool iop_full_uptodate(struct iomap_page *iop, unsigned int nrblocks) { return bitmap_full(iop->state, nrblocks); } static inline void iop_set_range_dirty(struct iomap_page *iop, unsigned int start, unsigned int len, unsigned int nrblocks) { bitmap_set(iop->state, start + nrblocks, len); } static inline void iop_clear_range_dirty(struct iomap_page *iop, unsigned int start, unsigned int len, unsigned int nrblocks) { bitmap_clear(iop->state, start + nrblocks, len); } static inline bool iop_test_dirty(struct iomap_page *iop, unsigned int pos, unsigned int nrblocks) { return test_bit(pos + nrblocks, iop->state); } -ritesh
diff --git a/fs/iomap/buffered-io.c b/fs/iomap/buffered-io.c index e9c85fcf7a1f..faee2852db8f 100644 --- a/fs/iomap/buffered-io.c +++ b/fs/iomap/buffered-io.c @@ -25,13 +25,13 @@ /* * Structure allocated for each folio when block size < folio size - * to track sub-folio uptodate status and I/O completions. + * to track sub-folio uptodate state and I/O completions. */ struct iomap_page { atomic_t read_bytes_pending; atomic_t write_bytes_pending; - spinlock_t uptodate_lock; - unsigned long uptodate[]; + spinlock_t state_lock; + unsigned long state[]; }; static inline struct iomap_page *to_iomap_page(struct folio *folio) @@ -58,12 +58,12 @@ iomap_page_create(struct inode *inode, struct folio *folio, unsigned int flags) else gfp = GFP_NOFS | __GFP_NOFAIL; - iop = kzalloc(struct_size(iop, uptodate, BITS_TO_LONGS(nr_blocks)), + iop = kzalloc(struct_size(iop, state, BITS_TO_LONGS(nr_blocks)), gfp); if (iop) { - spin_lock_init(&iop->uptodate_lock); + spin_lock_init(&iop->state_lock); if (folio_test_uptodate(folio)) - bitmap_fill(iop->uptodate, nr_blocks); + bitmap_fill(iop->state, nr_blocks); folio_attach_private(folio, iop); } return iop; @@ -79,7 +79,7 @@ static void iomap_page_release(struct folio *folio) return; WARN_ON_ONCE(atomic_read(&iop->read_bytes_pending)); WARN_ON_ONCE(atomic_read(&iop->write_bytes_pending)); - WARN_ON_ONCE(bitmap_full(iop->uptodate, nr_blocks) != + WARN_ON_ONCE(bitmap_full(iop->state, nr_blocks) != folio_test_uptodate(folio)); kfree(iop); } @@ -110,7 +110,7 @@ static void iomap_adjust_read_range(struct inode *inode, struct folio *folio, /* move forward for each leading block marked uptodate */ for (i = first; i <= last; i++) { - if (!test_bit(i, iop->uptodate)) + if (!test_bit(i, iop->state)) break; *pos += block_size; poff += block_size; @@ -120,7 +120,7 @@ static void iomap_adjust_read_range(struct inode *inode, struct folio *folio, /* truncate len if we find any trailing uptodate block(s) */ for ( ; i <= last; i++) { - if (test_bit(i, iop->uptodate)) { + if (test_bit(i, iop->state)) { plen -= (last - i + 1) * block_size; last = i - 1; break; @@ -152,11 +152,11 @@ static void iomap_iop_set_range_uptodate(struct folio *folio, unsigned last = (off + len - 1) >> inode->i_blkbits; unsigned long flags; - spin_lock_irqsave(&iop->uptodate_lock, flags); - bitmap_set(iop->uptodate, first, last - first + 1); - if (bitmap_full(iop->uptodate, i_blocks_per_folio(inode, folio))) + spin_lock_irqsave(&iop->state_lock, flags); + bitmap_set(iop->state, first, last - first + 1); + if (bitmap_full(iop->state, i_blocks_per_folio(inode, folio))) folio_mark_uptodate(folio); - spin_unlock_irqrestore(&iop->uptodate_lock, flags); + spin_unlock_irqrestore(&iop->state_lock, flags); } static void iomap_set_range_uptodate(struct folio *folio, @@ -451,7 +451,7 @@ bool iomap_is_partially_uptodate(struct folio *folio, size_t from, size_t count) last = (from + count - 1) >> inode->i_blkbits; for (i = first; i <= last; i++) - if (!test_bit(i, iop->uptodate)) + if (!test_bit(i, iop->state)) return false; return true; } @@ -1606,7 +1606,7 @@ iomap_writepage_map(struct iomap_writepage_ctx *wpc, * invalid, grab a new one. */ for (i = 0; i < nblocks && pos < end_pos; i++, pos += len) { - if (iop && !test_bit(i, iop->uptodate)) + if (iop && !test_bit(i, iop->state)) continue; error = wpc->ops->map_blocks(wpc, inode, pos);
This patch just changes the struct iomap_page uptodate & uptodate_lock member names to state and state_lock to better reflect their purpose for the upcoming patch. Signed-off-by: Ritesh Harjani (IBM) <ritesh.list@gmail.com> --- fs/iomap/buffered-io.c | 30 +++++++++++++++--------------- 1 file changed, 15 insertions(+), 15 deletions(-)