On Fri, Jul 17, 2020 at 02:10:18AM -0600, Andreas Dilger wrote:
> 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!");
Ok, I'll change it. Explicitely saying "journal device" makes it even
more clear to me.
+ "filesystem has both journal inode and journal 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'");
Yeah, that's better, thanks.
>
> > @@ -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.
True, it makes it kind of redundant when we've already printed the
error.
Thanks Andreas, I'll resend the new version.
-Lukas
>
>
> Cheers, Andreas
>
>
>
>
>
