[PATCH] xfs: don't allow log writes if the data device is readonly

[PATCH] xfs: don't allow log writes if the data device is readonly

[Darrick J. Wong]

From: Darrick J. Wong <djwong@kernel.org>

While running generic/050 with an external log, I observed this warning
in dmesg:

Trying to write to read-only block-device sda4 (partno 4)
WARNING: CPU: 2 PID: 215677 at block/blk-core.c:704 submit_bio_checks+0x256/0x510
Call Trace:
 submit_bio_noacct+0x2c/0x430
 _xfs_buf_ioapply+0x283/0x3c0 [xfs]
 __xfs_buf_submit+0x6a/0x210 [xfs]
 xfs_buf_delwri_submit_buffers+0xf8/0x270 [xfs]
 xfsaild+0x2db/0xc50 [xfs]
 kthread+0x14b/0x170

I think this happened because we tried to cover the log after a readonly
mount, and the AIL tried to write the primary superblock to the data
device.  The test marks the data device readonly, but it doesn't do the
same to the external log device.  Therefore, XFS thinks that the log is
writable, even though AIL writes whine to dmesg because the data device
is read only.

Fix this by amending xfs_log_writable to prevent writes when the AIL
can't possible write anything into the filesystem.

Note: As for the external log or the rt devices being readonly--
xfs_blkdev_get will complain about that if we aren't doing a norecovery
mount.

Signed-off-by: Darrick J. Wong <djwong@kernel.org>
---
 fs/xfs/xfs_log.c |    4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/fs/xfs/xfs_log.c b/fs/xfs/xfs_log.c
index 06041834daa3..e4839f22ec07 100644
--- a/fs/xfs/xfs_log.c
+++ b/fs/xfs/xfs_log.c
@@ -358,12 +358,14 @@ xfs_log_writable(
 	 * Never write to the log on norecovery mounts, if the block device is
 	 * read-only, or if the filesystem is shutdown. Read-only mounts still
 	 * allow internal writes for log recovery and unmount purposes, so don't
-	 * restrict that case here.
+	 * restrict that case here unless the data device is also readonly.
 	 */
 	if (mp->m_flags & XFS_MOUNT_NORECOVERY)
 		return false;
 	if (xfs_readonly_buftarg(mp->m_log->l_targ))
 		return false;
+	if (xfs_readonly_buftarg(mp->m_ddev_targp))
+		return false;
 	if (XFS_FORCED_SHUTDOWN(mp))
 		return false;
 	return true;

Re: [PATCH] xfs: don't allow log writes if the data device is readonly

[Brian Foster]

On Thu, Apr 29, 2021 at 05:40:12PM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <djwong@kernel.org>
> 
> While running generic/050 with an external log, I observed this warning
> in dmesg:
> 
> Trying to write to read-only block-device sda4 (partno 4)
> WARNING: CPU: 2 PID: 215677 at block/blk-core.c:704 submit_bio_checks+0x256/0x510
> Call Trace:
>  submit_bio_noacct+0x2c/0x430
>  _xfs_buf_ioapply+0x283/0x3c0 [xfs]
>  __xfs_buf_submit+0x6a/0x210 [xfs]
>  xfs_buf_delwri_submit_buffers+0xf8/0x270 [xfs]
>  xfsaild+0x2db/0xc50 [xfs]
>  kthread+0x14b/0x170
> 
> I think this happened because we tried to cover the log after a readonly
> mount, and the AIL tried to write the primary superblock to the data
> device.  The test marks the data device readonly, but it doesn't do the
> same to the external log device.  Therefore, XFS thinks that the log is
> writable, even though AIL writes whine to dmesg because the data device
> is read only.
> 
> Fix this by amending xfs_log_writable to prevent writes when the AIL
> can't possible write anything into the filesystem.
> 
> Note: As for the external log or the rt devices being readonly--
> xfs_blkdev_get will complain about that if we aren't doing a norecovery
> mount.
> 
> Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> ---
>  fs/xfs/xfs_log.c |    4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/xfs/xfs_log.c b/fs/xfs/xfs_log.c
> index 06041834daa3..e4839f22ec07 100644
> --- a/fs/xfs/xfs_log.c
> +++ b/fs/xfs/xfs_log.c
> @@ -358,12 +358,14 @@ xfs_log_writable(
>  	 * Never write to the log on norecovery mounts, if the block device is
>  	 * read-only, or if the filesystem is shutdown. Read-only mounts still
>  	 * allow internal writes for log recovery and unmount purposes, so don't
> -	 * restrict that case here.
> +	 * restrict that case here unless the data device is also readonly.
>  	 */

The comment update is a little confusing because that second sentence
explicitly refers to the read-only mount case (i.e., why we don't check
XFS_MOUNT_RDONLY here), and that logic/reasoning remains independent of
this change. Perhaps instead change the first part to something like
"... if the data or log device is read-only, ..." ?

With that fixed up:

Reviewed-by: Brian Foster <bfoster@redhat.com>

>  	if (mp->m_flags & XFS_MOUNT_NORECOVERY)
>  		return false;
>  	if (xfs_readonly_buftarg(mp->m_log->l_targ))
>  		return false;
> +	if (xfs_readonly_buftarg(mp->m_ddev_targp))
> +		return false;
>  	if (XFS_FORCED_SHUTDOWN(mp))
>  		return false;
>  	return true;
> 

Re: [PATCH] xfs: don't allow log writes if the data device is readonly

[Darrick J. Wong]

On Fri, Apr 30, 2021 at 09:32:48AM -0400, Brian Foster wrote:
> On Thu, Apr 29, 2021 at 05:40:12PM -0700, Darrick J. Wong wrote:
> > From: Darrick J. Wong <djwong@kernel.org>
> > 
> > While running generic/050 with an external log, I observed this warning
> > in dmesg:
> > 
> > Trying to write to read-only block-device sda4 (partno 4)
> > WARNING: CPU: 2 PID: 215677 at block/blk-core.c:704 submit_bio_checks+0x256/0x510
> > Call Trace:
> >  submit_bio_noacct+0x2c/0x430
> >  _xfs_buf_ioapply+0x283/0x3c0 [xfs]
> >  __xfs_buf_submit+0x6a/0x210 [xfs]
> >  xfs_buf_delwri_submit_buffers+0xf8/0x270 [xfs]
> >  xfsaild+0x2db/0xc50 [xfs]
> >  kthread+0x14b/0x170
> > 
> > I think this happened because we tried to cover the log after a readonly
> > mount, and the AIL tried to write the primary superblock to the data
> > device.  The test marks the data device readonly, but it doesn't do the
> > same to the external log device.  Therefore, XFS thinks that the log is
> > writable, even though AIL writes whine to dmesg because the data device
> > is read only.
> > 
> > Fix this by amending xfs_log_writable to prevent writes when the AIL
> > can't possible write anything into the filesystem.
> > 
> > Note: As for the external log or the rt devices being readonly--
> > xfs_blkdev_get will complain about that if we aren't doing a norecovery
> > mount.
> > 
> > Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> > ---
> >  fs/xfs/xfs_log.c |    4 +++-
> >  1 file changed, 3 insertions(+), 1 deletion(-)
> > 
> > diff --git a/fs/xfs/xfs_log.c b/fs/xfs/xfs_log.c
> > index 06041834daa3..e4839f22ec07 100644
> > --- a/fs/xfs/xfs_log.c
> > +++ b/fs/xfs/xfs_log.c
> > @@ -358,12 +358,14 @@ xfs_log_writable(
> >  	 * Never write to the log on norecovery mounts, if the block device is
> >  	 * read-only, or if the filesystem is shutdown. Read-only mounts still
> >  	 * allow internal writes for log recovery and unmount purposes, so don't
> > -	 * restrict that case here.
> > +	 * restrict that case here unless the data device is also readonly.
> >  	 */
> 
> The comment update is a little confusing because that second sentence
> explicitly refers to the read-only mount case (i.e., why we don't check
> XFS_MOUNT_RDONLY here), and that logic/reasoning remains independent of
> this change. Perhaps instead change the first part to something like
> "... if the data or log device is read-only, ..." ?

Will do.  Thanks for the review!

--D

> With that fixed up:
> 
> Reviewed-by: Brian Foster <bfoster@redhat.com>
> 
> >  	if (mp->m_flags & XFS_MOUNT_NORECOVERY)
> >  		return false;
> >  	if (xfs_readonly_buftarg(mp->m_log->l_targ))
> >  		return false;
> > +	if (xfs_readonly_buftarg(mp->m_ddev_targp))
> > +		return false;
> >  	if (XFS_FORCED_SHUTDOWN(mp))
> >  		return false;
> >  	return true;
> > 
> 

[PATCH v2] xfs: don't allow log writes if the data device is readonly

[Darrick J. Wong]

From: Darrick J. Wong <djwong@kernel.org>

While running generic/050 with an external log, I observed this warning
in dmesg:

Trying to write to read-only block-device sda4 (partno 4)
WARNING: CPU: 2 PID: 215677 at block/blk-core.c:704 submit_bio_checks+0x256/0x510
Call Trace:
 submit_bio_noacct+0x2c/0x430
 _xfs_buf_ioapply+0x283/0x3c0 [xfs]
 __xfs_buf_submit+0x6a/0x210 [xfs]
 xfs_buf_delwri_submit_buffers+0xf8/0x270 [xfs]
 xfsaild+0x2db/0xc50 [xfs]
 kthread+0x14b/0x170

I think this happened because we tried to cover the log after a readonly
mount, and the AIL tried to write the primary superblock to the data
device.  The test marks the data device readonly, but it doesn't do the
same to the external log device.  Therefore, XFS thinks that the log is
writable, even though AIL writes whine to dmesg because the data device
is read only.

Fix this by amending xfs_log_writable to prevent writes when the AIL
can't possible write anything into the filesystem.

Note: As for the external log or the rt devices being readonly--
xfs_blkdev_get will complain about that if we aren't doing a norecovery
mount.

Signed-off-by: Darrick J. Wong <djwong@kernel.org>
Reviewed-by: Brian Foster <bfoster@redhat.com>
---
v2: tweak the wording of the comment a little bit
---
 fs/xfs/xfs_log.c |   10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/fs/xfs/xfs_log.c b/fs/xfs/xfs_log.c
index 06041834daa3..c19a82adea1e 100644
--- a/fs/xfs/xfs_log.c
+++ b/fs/xfs/xfs_log.c
@@ -355,13 +355,15 @@ xfs_log_writable(
 	struct xfs_mount	*mp)
 {
 	/*
-	 * Never write to the log on norecovery mounts, if the block device is
-	 * read-only, or if the filesystem is shutdown. Read-only mounts still
-	 * allow internal writes for log recovery and unmount purposes, so don't
-	 * restrict that case here.
+	 * Do not write to the log on norecovery mounts, if the data or log
+	 * devices are read-only, or if the filesystem is shutdown. Read-only
+	 * mounts allow internal writes for log recovery and unmount purposes,
+	 * so don't restrict that case.
 	 */
 	if (mp->m_flags & XFS_MOUNT_NORECOVERY)
 		return false;
+	if (xfs_readonly_buftarg(mp->m_ddev_targp))
+		return false;
 	if (xfs_readonly_buftarg(mp->m_log->l_targ))
 		return false;
 	if (XFS_FORCED_SHUTDOWN(mp))