On Thu, May 13, 2021 at 02:09:26PM -0400, Theodore Ts'o wrote:
> > +
> > + err = jbd2_journal_bmap(journal, log_offset, &block_start);
> > + if (err) {
> > + printk(KERN_ERR "JBD2: bad block at offset %lu", log_offset);
> > + return err;
> > + }
>
> We could get rid of this, and instead make sure block_start is initialized
> to ~((unsigned long long) 0). Then in the loop we can do...
>
> > +
> > + /*
> > + * use block_start - 1 to meet check for contiguous with previous region:
> > + * phys_block == block_stop + 1
> > + */
> > + block_stop = block_start - 1;
> > +
> > + for (block = log_offset; block < journal->j_total_len; block++) {
> > + err = jbd2_journal_bmap(journal, block, &phys_block);
> > + if (err) {
> > + printk(KERN_ERR "JBD2: bad block at offset %lu", block);
> > + return err;
> > + }
>
> if (block_start == ~((unsigned long long) 0)) {
> block_start = phys_block;
> block_Stop = block_start - 1;
> }
>
> > +
> > + if (block == journal->j_total_len - 1) {
> > + block_stop = phys_block;
> > + } else if (phys_block == block_stop + 1) {
> > + block_stop++;
> > + continue;
> > + }
> > +
> > + /*
> > + * not contiguous with prior physical block or this is last
> > + * block of journal, take care of the region
> > + */
> > + byte_start = block_start * journal->j_blocksize;
> > + byte_stop = block_stop * journal->j_blocksize;
> > + byte_count = (block_stop - block_start + 1) *
> > + journal->j_blocksize;
> > +
> > + truncate_inode_pages_range(journal->j_dev->bd_inode->i_mapping,
> > + byte_start, byte_stop);
> > +
> > + if (flags & JBD2_ERASE_FLAG_DISCARD) {
> > + err = blkdev_issue_discard(journal->j_dev,
> > + byte_start >> SECTOR_SHIFT,
> > + byte_count >> SECTOR_SHIFT,
> > + GFP_NOFS, 0);
> > + } else if (flags & JBD2_ERASE_FLAG_ZEROOUT) {
> > + err = blkdev_issue_zeroout(journal->j_dev,
> > + byte_start >> SECTOR_SHIFT,
> > + byte_count >> SECTOR_SHIFT,
> > + GFP_NOFS, 0);
> > + }
> > +
> > + if (unlikely(err != 0)) {
> > + printk(KERN_ERR "JBD2: (error %d) unable to wipe journal at physical blocks %llu - %llu",
> > + err, block_start, block_stop);
> > + return err;
> > + }
> > +
> > + block_start = phys_block;
> > + block_stop = phys_block;
>
> Is this right? When we initialized the loop, above, block_stop was
> set to block_start-1 (where block_start == phys_block). So I think it
> might be more correct to replace the above two lines with:
>
> block_start = ~((unsigned long long) 0);
>
> ... and then let block_start and block_stop be initialized in a single
> place. Do you agree? Does this make sense to you?
I just tried this and it actually wouldn't work with this setup because
phys_block would be set after the new call to bmap instead of keeping the value
from the end of the prior loop. However, I have reworked the code using the
general idea of the block_start reset you proposed and I will submit this in
the next version.
Thanks,
Leah
>
> - Ted
