On Mon, Jun 5, 2023 at 9:33 AM Matthew Wilcox <willy@xxxxxxxxxxxxx> wrote:
>
> On Mon, Jun 05, 2023 at 07:01:52AM +0530, Ritesh Harjani (IBM) wrote:
> > +static void iop_set_range_dirty(struct inode *inode, struct folio *folio,
> > + size_t off, size_t len)
> > +{
> > + struct iomap_page *iop = to_iomap_page(folio);
> > + unsigned int blks_per_folio = i_blocks_per_folio(inode, folio);
> > + unsigned int first_blk = off >> inode->i_blkbits;
> > + unsigned int last_blk = (off + len - 1) >> inode->i_blkbits;
> > + unsigned int nr_blks = last_blk - first_blk + 1;
> > + unsigned long flags;
> > +
> > + spin_lock_irqsave(&iop->state_lock, flags);
> > + bitmap_set(iop->state, first_blk + blks_per_folio, nr_blks);
> > + spin_unlock_irqrestore(&iop->state_lock, flags);
> > +}
> > +
> > +static void iomap_iop_set_range_dirty(struct inode *inode, struct folio *folio,
> > + size_t off, size_t len)
> > +{
> > + struct iomap_page *iop = to_iomap_page(folio);
> > +
> > + if (iop)
> > + iop_set_range_dirty(inode, folio, off, len);
> > +}
>
> Why are these separate functions? It'd be much better written as:
It got discussed here [1] on the preference would be to have it in a
separate helper.
[1]: https://lore.kernel.org/linux-xfs/ZGYnzcoGuzWKa7lh@xxxxxxxxxxxxx/
>
> static void iomap_iop_set_range_dirty(struct inode *inode, struct folio *folio,
> size_t off, size_t len)
> {
> struct iomap_page *iop = to_iomap_page(folio);
> unsigned int start, first, last;
> unsigned long flags;
>
> if (!iop)
> return;
>
> start = i_blocks_per_folio(inode, folio);
> first = off >> inode->i_blkbits;
> last = (off + len - 1) >> inode->i_blkbits;
>
> spin_lock_irqsave(&iop->state_lock, flags);
> bitmap_set(iop->state, start + first, last - first + 1);
> spin_unlock_irqrestore(&iop->state_lock, flags);
> }
>
> > +static void iop_clear_range_dirty(struct inode *inode, struct folio *folio,
> > + size_t off, size_t len)
> > +{
> > + struct iomap_page *iop = to_iomap_page(folio);
> > + unsigned int blks_per_folio = i_blocks_per_folio(inode, folio);
> > + unsigned int first_blk = off >> inode->i_blkbits;
> > + unsigned int last_blk = (off + len - 1) >> inode->i_blkbits;
> > + unsigned int nr_blks = last_blk - first_blk + 1;
> > + unsigned long flags;
> > +
> > + spin_lock_irqsave(&iop->state_lock, flags);
> > + bitmap_clear(iop->state, first_blk + blks_per_folio, nr_blks);
> > + spin_unlock_irqrestore(&iop->state_lock, flags);
> > +}
> > +
> > +static void iomap_iop_clear_range_dirty(struct inode *inode,
> > + struct folio *folio, size_t off, size_t len)
> > +{
> > + struct iomap_page *iop = to_iomap_page(folio);
> > +
> > + if (iop)
> > + iop_clear_range_dirty(inode, folio, off, len);
> > +}
>
> Similarly
>
> > +bool iomap_dirty_folio(struct address_space *mapping, struct folio *folio)
> > +{
> > + struct iomap_page __maybe_unused *iop;
> > + struct inode *inode = mapping->host;
> > + size_t len = folio_size(folio);
> > +
> > + iop = iomap_iop_alloc(inode, folio, 0);
>
> Why do you keep doing this? Just throw away the return value from
> iomap_iop_alloc(). Don't clutter the source with the unnecessary
> variable declaration and annotation that it's not used!
>
Sorry, it got leftover. I will quickly fix this.
> > +static int iomap_write_delalloc_punch(struct inode *inode, struct folio *folio,
> > + loff_t *punch_start_byte, loff_t start_byte, loff_t end_byte,
> > + int (*punch)(struct inode *inode, loff_t offset, loff_t length))
> > +{
> > + struct iomap_page *iop;
> > + unsigned int first_blk, last_blk, i;
> > + loff_t last_byte;
> > + u8 blkbits = inode->i_blkbits;
> > + int ret = 0;
> > +
> > + if (start_byte > *punch_start_byte) {
> > + ret = punch(inode, *punch_start_byte,
> > + start_byte - *punch_start_byte);
> > + if (ret)
> > + goto out_err;
> > + }
> > + /*
> > + * When we have per-block dirty tracking, there can be
> > + * blocks within a folio which are marked uptodate
> > + * but not dirty. In that case it is necessary to punch
> > + * out such blocks to avoid leaking any delalloc blocks.
> > + */
> > + iop = to_iomap_page(folio);
> > + if (!iop)
> > + goto skip_iop_punch;
> > +
> > + last_byte = min_t(loff_t, end_byte - 1,
> > + (folio_next_index(folio) << PAGE_SHIFT) - 1);
> > + first_blk = offset_in_folio(folio, start_byte) >> blkbits;
> > + last_blk = offset_in_folio(folio, last_byte) >> blkbits;
> > + for (i = first_blk; i <= last_blk; i++) {
> > + if (!iop_test_block_dirty(folio, i)) {
> > + ret = punch(inode, i << blkbits, 1 << blkbits);
> > + if (ret)
> > + goto out_err;
> > + }
> > + }
> > +
> > +skip_iop_punch:
> > + /*
> > + * Make sure the next punch start is correctly bound to
> > + * the end of this data range, not the end of the folio.
> > + */
> > + *punch_start_byte = min_t(loff_t, end_byte,
> > + folio_next_index(folio) << PAGE_SHIFT);
> > +
> > + return ret;
> > +
> > +out_err:
> > + folio_unlock(folio);
> > + folio_put(folio);
> > + return ret;
> > +
> > +}
> > +
> > /*
> > * Scan the data range passed to us for dirty page cache folios. If we find a
> > * dirty folio, punch out the preceeding range and update the offset from which
> > @@ -940,26 +1074,9 @@ static int iomap_write_delalloc_scan(struct inode *inode,
> > }
> >
> > /* if dirty, punch up to offset */
> > - if (folio_test_dirty(folio)) {
> > - if (start_byte > *punch_start_byte) {
> > - int error;
> > -
> > - error = punch(inode, *punch_start_byte,
> > - start_byte - *punch_start_byte);
> > - if (error) {
> > - folio_unlock(folio);
> > - folio_put(folio);
> > - return error;
> > - }
> > - }
> > -
> > - /*
> > - * Make sure the next punch start is correctly bound to
> > - * the end of this data range, not the end of the folio.
> > - */
> > - *punch_start_byte = min_t(loff_t, end_byte,
> > - folio_next_index(folio) << PAGE_SHIFT);
> > - }
> > + if (folio_test_dirty(folio))
> > + iomap_write_delalloc_punch(inode, folio, punch_start_byte,
> > + start_byte, end_byte, punch);
> >
> > /* move offset to start of next folio in range */
> > start_byte = folio_next_index(folio) << PAGE_SHIFT;
>
> I'm having trouble following this refactoring + modification. Perhaps
> I'm just tired.
>
Let me refactor this part out in the next revision.
-ritesh
