On Jul 16, 2020, at 12:39 PM, Lukas Czerner <lczerner@xxxxxxxxxx> wrote:
>
> Ext4 uses blkdev_get_by_dev() to get the block_device for journal device
> which does check to see if the read-only block device was opened
> read-only.
>
> As a result ext4 will hapily proceed mounting the file system with
> external journal on read-only device. This is bad as we would not be
> able to use the journal leading to errors later on.
>
> Instead of simply failing to mount file system in this case, treat it in
> a similar way we treat internal journal on read-only device. Allow to
> mount with -o noload in read-only mode.
>
> This can be reproduced easily like this:
>
> mke2fs -F -O journal_dev $JOURNAL_DEV 100M
> mkfs.$FSTYPE -F -J device=$JOURNAL_DEV $FS_DEV
> blockdev --setro $JOURNAL_DEV
> mount $FS_DEV $MNT
> touch $MNT/file
> umount $MNT
>
> Signed-off-by: Lukas Czerner <lczerner@xxxxxxxxxx>
> ---
> fs/ext4/super.c | 55 ++++++++++++++++++++++++++++++-------------------
> 1 file changed, 34 insertions(+), 21 deletions(-)
>
> diff --git a/fs/ext4/super.c b/fs/ext4/super.c
> index 330957ed1f05..a15e3c751766 100644
> --- a/fs/ext4/super.c
> +++ b/fs/ext4/super.c
> @@ -5088,7 +5089,30 @@ static int ext4_load_journal(struct super_block *sb,
> } else
> journal_dev = new_decode_dev(le32_to_cpu(es->s_journal_dev));
>
> - really_read_only = bdev_read_only(sb->s_bdev);
> + if (journal_inum && journal_dev) {
> + ext4_msg(sb, KERN_ERR, "filesystem has both journal "
> + "and inode journals!");
(style) keep error string on a single line. Also, "journal and inode journal"
is not very clear what the problem is. Maybe something like:
+ ext4_msg(sb, KERN_ERR,
+ "filesystem has both journal inode and device!");
> + return -EINVAL;
> + }
> +
> + if (journal_inum) {
> + if (!(journal = ext4_get_journal(sb, journal_inum)))
> + return -EINVAL;
> + } else {
> + if (!(journal = ext4_get_dev_journal(sb, journal_dev)))
> + return -EINVAL;
> + }
> +
> + journal_dev_ro = bdev_read_only(journal->j_dev);
> + really_read_only = bdev_read_only(sb->s_bdev) | journal_dev_ro;
> +
> + if (journal_dev_ro && !sb_rdonly(sb)) {
> + ext4_msg(sb, KERN_ERR, "write access "
> + "unavailable, cannot proceed "
> + "(try mounting read-only)");
(style) should keep error strings on a single line. Also, this isn't very
obvious that that this is because of the read-only journal device. Maybe:
ext4_msg(sb, KERN_ERR,
"journal device read-only, try mounting with '-o ro'");
> @@ -5141,11 +5152,8 @@ static int ext4_load_journal(struct super_block *sb,
> kfree(save);
> }
>
> - if (err) {
> - ext4_msg(sb, KERN_ERR, "error loading journal");
> - jbd2_journal_destroy(journal);
> - return err;
> - }
> + if (err)
> + goto err_out;
>
> EXT4_SB(sb)->s_journal = journal;
> ext4_clear_journal_err(sb, es);
> @@ -5159,6 +5167,11 @@ static int ext4_load_journal(struct super_block *sb,
> }
>
> return 0;
> +
> +err_out:
> + ext4_msg(sb, KERN_ERR, "error loading journal");
Is there any error case that doesn't already print its own error message?
Maybe better to leave the ext4_msg() in the original location, and only
do cleanup here.
Cheers, Andreas
Attachment:
signature.asc
Description: Message signed with OpenPGP
