On Thu, Mar 25, 2021 at 06:21:46PM -0700, Darrick J. Wong wrote: > On Thu, Mar 25, 2021 at 06:12:20PM +0000, Leah Rumancik wrote: > > ioctl EXT4_FLUSH_JOURNAL flushes the journal log with the option to > > discard journal log blocks. With the filename wipeout patch, if the discard > > mount option is set, Ext4 guarantees that all data will be discarded on > > deletion. This ioctl allows for periodically discarding journal contents > > too. > > Hrm. What is the use case here? I guess the goal is to sanitize the > ondisk log contents (even as wiping deleted filenames becomes default) > every now and then? Why do we want cleaning up the log to be an > explicitly separate step that userspace has to invoke? > > (As opposed, say, to discarding the log automatically after every > journal checkpoint if a journal/mount option is set?) The goal here is to be able to ensure everything is sanitized at a particular point in time. If done automatically through the checkpoint, there is no guarantee as to how often / when the sanitizing is performed. > > > Also, add journal discard (if discard supported) during journal load > > after recovery. This provides a potential solution to > > https://lore.kernel.org/linux-ext4/YDZoaacIYStFQT8g@xxxxxxx/ for > > disks that support discard. After a successful journal recovery, e2fsck can > > call this ioctl to discard the journal as well. > > > > Signed-off-by: Leah Rumancik <leah.rumancik@xxxxxxxxx> > > --- > > fs/ext4/ext4.h | 1 + > > fs/ext4/ioctl.c | 28 +++++++++++ > > fs/jbd2/journal.c | 116 +++++++++++++++++++++++++++++++++++++++++-- > > include/linux/jbd2.h | 1 + > > 4 files changed, 143 insertions(+), 3 deletions(-) > > > > diff --git a/fs/ext4/ext4.h b/fs/ext4/ext4.h > > index 8011418176bc..92c039ebcba7 100644 > > --- a/fs/ext4/ext4.h > > +++ b/fs/ext4/ext4.h > > @@ -724,6 +724,7 @@ enum { > > #define EXT4_IOC_CLEAR_ES_CACHE _IO('f', 40) > > #define EXT4_IOC_GETSTATE _IOW('f', 41, __u32) > > #define EXT4_IOC_GET_ES_CACHE _IOWR('f', 42, struct fiemap) > > +#define EXT4_FLUSH_JOURNAL _IOW('f', 43, int) > > > > #define EXT4_IOC_SHUTDOWN _IOR ('X', 125, __u32) > > > > diff --git a/fs/ext4/ioctl.c b/fs/ext4/ioctl.c > > index a2cf35066f46..1d3636c1de3b 100644 > > --- a/fs/ext4/ioctl.c > > +++ b/fs/ext4/ioctl.c > > @@ -1318,6 +1318,33 @@ static long __ext4_ioctl(struct file *filp, unsigned int cmd, unsigned long arg) > > return -EOPNOTSUPP; > > return fsverity_ioctl_read_metadata(filp, > > (const void __user *)arg); > > + case EXT4_FLUSH_JOURNAL: > > + { > > + int discard = 0, err = 0; > > + > > + /* file argument is not the mount point */ > > + if (file_dentry(filp) != sb->s_root) > > + return -EINVAL; > > + > > + /* filesystem is not backed by block device */ > > + if (sb->s_bdev == NULL) > > + return -EINVAL; > > + > > + if (copy_from_user(&discard, (int __user *)arg, sizeof(int))) > > + return -EFAULT; > > + > > + if (EXT4_SB(sb)->s_journal) { > > + jbd2_journal_lock_updates(EXT4_SB(sb)->s_journal); > > + > > + if (discard) > > + err = jbd2_journal_flush_and_discard(EXT4_SB(sb)->s_journal); > > + else > > + err = jbd2_journal_flush(EXT4_SB(sb)->s_journal); > > Why not pass this as a flag? > > --D > Yes sure, that would make it simpler. I will update it. > > + > > + jbd2_journal_unlock_updates(EXT4_SB(sb)->s_journal); > > + } > > + return err; > > + } > > > > default: > > return -ENOTTY; > > @@ -1407,6 +1434,7 @@ long ext4_compat_ioctl(struct file *file, unsigned int cmd, unsigned long arg) > > case EXT4_IOC_GET_ES_CACHE: > > case FS_IOC_FSGETXATTR: > > case FS_IOC_FSSETXATTR: > > + case EXT4_FLUSH_JOURNAL: > > break; > > default: > > return -ENOIOCTLCMD; > > diff --git a/fs/jbd2/journal.c b/fs/jbd2/journal.c > > index 2dc944442802..9718512e7178 100644 > > --- a/fs/jbd2/journal.c > > +++ b/fs/jbd2/journal.c > > @@ -67,6 +67,7 @@ EXPORT_SYMBOL(jbd2_journal_set_triggers); > > EXPORT_SYMBOL(jbd2_journal_dirty_metadata); > > EXPORT_SYMBOL(jbd2_journal_forget); > > EXPORT_SYMBOL(jbd2_journal_flush); > > +EXPORT_SYMBOL(jbd2_journal_flush_and_discard); > > EXPORT_SYMBOL(jbd2_journal_revoke); > > > > EXPORT_SYMBOL(jbd2_journal_init_dev); > > @@ -1686,6 +1687,90 @@ static void jbd2_mark_journal_empty(journal_t *journal, int write_op) > > write_unlock(&journal->j_state_lock); > > } > > > > +/* discard journal blocks excluding journal superblock */ > > +static int __jbd2_journal_issue_discard(journal_t *journal) > > +{ > > + int err = 0; > > + unsigned long block, log_offset; > > + unsigned long long phys_block, block_start, block_stop; > > + loff_t byte_start, byte_stop, byte_count; > > + struct request_queue *q = bdev_get_queue(journal->j_dev); > > + > > + if (!q) > > + return -ENXIO; > > + > > + if (!blk_queue_discard(q)) > > + return -EOPNOTSUPP; > > + > > + /* lookup block mapping and issue discard for each contiguous region */ > > + log_offset = be32_to_cpu(journal->j_superblock->s_first); > > + > > + err = jbd2_journal_bmap(journal, log_offset, &block_start); > > + if (err) { > > + printk(KERN_ERR "JBD2: bad block at offset %lu", log_offset); > > + return err; > > + } > > + > > + /* > > + * 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 is last block, update stopping point > > + * if not last block and > > + * block is contiguous with previous block, continue > > + */ > > + if (block == journal->j_total_len - 1) > > + block_stop = phys_block; > > + else if (phys_block == block_stop + 1) { > > + block_stop++; > > + continue; > > + } > > + > > + /* > > + * if 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); > > + > > + /* > > + * use blkdev_issue_discard instead of sb_issue_discard > > + * because superblock not yet populated when this is > > + * called during journal_load during mount process > > + */ > > + err = blkdev_issue_discard(journal->j_dev, > > + byte_start >> SECTOR_SHIFT, > > + byte_count >> SECTOR_SHIFT, > > + GFP_NOFS, 0); > > + > > + if (unlikely(err != 0)) { > > + printk(KERN_ERR "JBD2: unable to discard " > > + "journal at physical blocks %llu - %llu", > > + block_start, block_stop); > > + return err; > > + } > > + > > + block_start = phys_block; > > + block_stop = phys_block; > > + } > > + > > + return blkdev_issue_flush(journal->j_dev); > > +} > > > > /** > > * jbd2_journal_update_sb_errno() - Update error in the journal. > > @@ -1892,6 +1977,7 @@ int jbd2_journal_load(journal_t *journal) > > { > > int err; > > journal_superblock_t *sb; > > + struct request_queue *q = bdev_get_queue(journal->j_dev); > > > > err = load_superblock(journal); > > if (err) > > @@ -1936,6 +2022,12 @@ int jbd2_journal_load(journal_t *journal) > > */ > > journal->j_flags &= ~JBD2_ABORT; > > > > + /* if journal device supports discard, discard journal blocks */ > > + if (q && blk_queue_discard(q)) { > > + if (__jbd2_journal_issue_discard(journal)) > > + printk(KERN_ERR "JBD2: failed to discard journal when loading"); > > + } > > + > > /* OK, we've finished with the dynamic journal bits: > > * reinitialise the dynamic contents of the superblock in memory > > * and reset them on disk. */ > > @@ -2244,15 +2336,18 @@ void jbd2_journal_clear_features(journal_t *journal, unsigned long compat, > > EXPORT_SYMBOL(jbd2_journal_clear_features); > > > > /** > > - * jbd2_journal_flush() - Flush journal > > + * __jbd2_journal_flush() - Flush journal > > * @journal: Journal to act on. > > + * @discard: flag (see below) > > * > > * Flush all data for a given journal to disk and empty the journal. > > * Filesystems can use this when remounting readonly to ensure that > > * recovery does not need to happen on remount. > > + * > > + * If 'discard' is false, the journal is simply flushed. If discard is true, > > + * the journal is also discarded. > > */ > > - > > -int jbd2_journal_flush(journal_t *journal) > > +static int __jbd2_journal_flush(journal_t *journal, bool discard) > > { > > int err = 0; > > transaction_t *transaction = NULL; > > @@ -2306,6 +2401,10 @@ int jbd2_journal_flush(journal_t *journal) > > * commits of data to the journal will restore the current > > * s_start value. */ > > jbd2_mark_journal_empty(journal, REQ_SYNC | REQ_FUA); > > + > > + if (discard) > > + err = __jbd2_journal_issue_discard(journal); > > + > > mutex_unlock(&journal->j_checkpoint_mutex); > > write_lock(&journal->j_state_lock); > > J_ASSERT(!journal->j_running_transaction); > > @@ -2318,6 +2417,17 @@ int jbd2_journal_flush(journal_t *journal) > > return err; > > } > > > > +int jbd2_journal_flush(journal_t *journal) > > +{ > > + return __jbd2_journal_flush(journal, false /* don't discard */); > > +} > > + > > +/* flush journal and discard journal log */ > > +int jbd2_journal_flush_and_discard(journal_t *journal) > > +{ > > + return __jbd2_journal_flush(journal, true /* also discard */); > > +} > > + > > /** > > * jbd2_journal_wipe() - Wipe journal contents > > * @journal: Journal to act on. > > diff --git a/include/linux/jbd2.h b/include/linux/jbd2.h > > index 99d3cd051ac3..9bed34e9a273 100644 > > --- a/include/linux/jbd2.h > > +++ b/include/linux/jbd2.h > > @@ -1492,6 +1492,7 @@ extern int jbd2_journal_invalidatepage(journal_t *, > > extern int jbd2_journal_try_to_free_buffers(journal_t *journal, struct page *page); > > extern int jbd2_journal_stop(handle_t *); > > extern int jbd2_journal_flush (journal_t *); > > +extern int jbd2_journal_flush_and_discard(journal_t *journal); > > extern void jbd2_journal_lock_updates (journal_t *); > > extern void jbd2_journal_unlock_updates (journal_t *); > > > > -- > > 2.31.0.291.g576ba9dcdaf-goog > > Thanks for the comments :)
