Re: [PATCH] ext4: handle read only external journal device

[Lukas Czerner <lczerner@redhat.com>]

On Fri, Jul 17, 2020 at 02:10:18AM -0600, Andreas Dilger wrote:
> On Jul 16, 2020, at 12:39 PM, Lukas Czerner <lczerner@redhat.com> 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@redhat.com>
> > ---
> > 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
> 
> 
> 
> 
>