[PATCH 0/1] RFE: Do not check NEEDSREPAIR if ro,norecovery mount.

[PATCH 0/1] RFE: Do not check NEEDSREPAIR if ro,norecovery mount.

[Lukas Herbolt]

Hi,
I disccussed this shortly with Eric Sandeen. I have came across 
corruption where xfs_repair fails and the only way how to get at
least some of the data is to use manually clear out the NEEDSREPAIR
flag. I do not think there is needs of checking if we are about
to mount the FS with norecovery,ro.

Lukas Herbolt (1):
  xfs: do not check NEEDSREPAIR if ro,norecovery mount.

 fs/xfs/xfs_super.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

-- 
2.48.1

[PATCH 1/1] xfs: do not check NEEDSREPAIR if ro,norecovery mount.

[Lukas Herbolt]

If there is corrutpion on the filesystem andxfs_repair
fails to repair it. The last resort of getting the data
is to use norecovery,ro mount. But if the NEEDSREPAIR is
set the filesystem cannot be mounted. The flag must be
cleared out manually using xfs_db, to get access to what
left over of the corrupted fs.

Signed-off-by: Lukas Herbolt <lukas@herbolt.com>
---
 fs/xfs/xfs_super.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c
index 394fdf3bb535..c2566dcc4f88 100644
--- a/fs/xfs/xfs_super.c
+++ b/fs/xfs/xfs_super.c
@@ -1635,8 +1635,12 @@ xfs_fs_fill_super(
 #endif
 	}
 
-	/* Filesystem claims it needs repair, so refuse the mount. */
-	if (xfs_has_needsrepair(mp)) {
+	/*
+	 * Filesystem claims it needs repair, so refuse the mount unless
+	 * norecovery is also specified, in which case the filesystem can
+	 * be mounted with no risk of further damage.
+	 */
+	if (xfs_has_needsrepair(mp) && !xfs_has_norecovery(mp)) {
 		xfs_warn(mp, "Filesystem needs repair.  Please run xfs_repair.");
 		error = -EFSCORRUPTED;
 		goto out_free_sb;
-- 
2.48.1

Re: [PATCH 1/1] xfs: do not check NEEDSREPAIR if ro,norecovery mount.

[Darrick J. Wong]

On Mon, Feb 03, 2025 at 09:55:13AM +0100, Lukas Herbolt wrote:
> If there is corrutpion on the filesystem andxfs_repair
> fails to repair it. The last resort of getting the data
> is to use norecovery,ro mount. But if the NEEDSREPAIR is
> set the filesystem cannot be mounted. The flag must be
> cleared out manually using xfs_db, to get access to what
> left over of the corrupted fs.
> 
> Signed-off-by: Lukas Herbolt <lukas@herbolt.com>
> ---
>  fs/xfs/xfs_super.c | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c
> index 394fdf3bb535..c2566dcc4f88 100644
> --- a/fs/xfs/xfs_super.c
> +++ b/fs/xfs/xfs_super.c
> @@ -1635,8 +1635,12 @@ xfs_fs_fill_super(
>  #endif
>  	}
>  
> -	/* Filesystem claims it needs repair, so refuse the mount. */
> -	if (xfs_has_needsrepair(mp)) {
> +	/*
> +	 * Filesystem claims it needs repair, so refuse the mount unless
> +	 * norecovery is also specified, in which case the filesystem can
> +	 * be mounted with no risk of further damage.
> +	 */
> +	if (xfs_has_needsrepair(mp) && !xfs_has_norecovery(mp)) {

I think a better way to handle badly damaged filesystems is for us to
provide a means to extract directory trees in userspace, rather than
making the user take the risk of mounting a known-bad filesystem.
I've a draft of an xfs_db subcommand for doing exactly that and will
share for xfsprogs 6.14.

--D

>  		xfs_warn(mp, "Filesystem needs repair.  Please run xfs_repair.");
>  		error = -EFSCORRUPTED;
>  		goto out_free_sb;
> -- 
> 2.48.1
> 
> 

Re: [PATCH 1/1] xfs: do not check NEEDSREPAIR if ro,norecovery mount.

[Eric Sandeen]

On 2/3/25 4:26 PM, Darrick J. Wong wrote:
> On Mon, Feb 03, 2025 at 09:55:13AM +0100, Lukas Herbolt wrote:
>> If there is corrutpion on the filesystem andxfs_repair
>> fails to repair it. The last resort of getting the data
>> is to use norecovery,ro mount. But if the NEEDSREPAIR is
>> set the filesystem cannot be mounted. The flag must be
>> cleared out manually using xfs_db, to get access to what
>> left over of the corrupted fs.
>>
>> Signed-off-by: Lukas Herbolt <lukas@herbolt.com>
>> ---
>>  fs/xfs/xfs_super.c | 8 ++++++--
>>  1 file changed, 6 insertions(+), 2 deletions(-)
>>
>> diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c
>> index 394fdf3bb535..c2566dcc4f88 100644
>> --- a/fs/xfs/xfs_super.c
>> +++ b/fs/xfs/xfs_super.c
>> @@ -1635,8 +1635,12 @@ xfs_fs_fill_super(
>>  #endif
>>  	}
>>  
>> -	/* Filesystem claims it needs repair, so refuse the mount. */
>> -	if (xfs_has_needsrepair(mp)) {
>> +	/*
>> +	 * Filesystem claims it needs repair, so refuse the mount unless
>> +	 * norecovery is also specified, in which case the filesystem can
>> +	 * be mounted with no risk of further damage.
>> +	 */
>> +	if (xfs_has_needsrepair(mp) && !xfs_has_norecovery(mp)) {
> 
> I think a better way to handle badly damaged filesystems is for us to
> provide a means to extract directory trees in userspace, rather than
> making the user take the risk of mounting a known-bad filesystem.
> I've a draft of an xfs_db subcommand for doing exactly that and will
> share for xfsprogs 6.14.

I think whether a userspace extractor is better or not depends on the
usecase. I suppose there's some truth that a NEEDSREPAIR filesystem is
"known bad" but we already suffer the risk of "unknown bad" filesystems
today. (Or for that matter, the fact that we allow "norecovery" today,
which also guarantees a mount of an inconsistent filesystem.)

"Something is wrong with this filesystem, let's mount it readonly and
copy off the data" is a pretty time-tested approach, I think, hampered
only by the fairly recent addition of NEEDSREPAIR.

a userspace scrape-the-device tool may well be useful for some, but
I don't think that vs. this kernelspace option needs to be an either/or
decision.

Thanks,

-Eric

> --D
> 
>>  		xfs_warn(mp, "Filesystem needs repair.  Please run xfs_repair.");
>>  		error = -EFSCORRUPTED;
>>  		goto out_free_sb;
>> -- 
>> 2.48.1
>>
>>
> 

Re: [PATCH 1/1] xfs: do not check NEEDSREPAIR if ro,norecovery mount.

[Darrick J. Wong]

On Tue, Feb 04, 2025 at 11:55:00AM -0600, Eric Sandeen wrote:
> On 2/3/25 4:26 PM, Darrick J. Wong wrote:
> > On Mon, Feb 03, 2025 at 09:55:13AM +0100, Lukas Herbolt wrote:
> >> If there is corrutpion on the filesystem andxfs_repair
> >> fails to repair it. The last resort of getting the data
> >> is to use norecovery,ro mount. But if the NEEDSREPAIR is
> >> set the filesystem cannot be mounted. The flag must be
> >> cleared out manually using xfs_db, to get access to what
> >> left over of the corrupted fs.
> >>
> >> Signed-off-by: Lukas Herbolt <lukas@herbolt.com>
> >> ---
> >>  fs/xfs/xfs_super.c | 8 ++++++--
> >>  1 file changed, 6 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c
> >> index 394fdf3bb535..c2566dcc4f88 100644
> >> --- a/fs/xfs/xfs_super.c
> >> +++ b/fs/xfs/xfs_super.c
> >> @@ -1635,8 +1635,12 @@ xfs_fs_fill_super(
> >>  #endif
> >>  	}
> >>  
> >> -	/* Filesystem claims it needs repair, so refuse the mount. */
> >> -	if (xfs_has_needsrepair(mp)) {
> >> +	/*
> >> +	 * Filesystem claims it needs repair, so refuse the mount unless
> >> +	 * norecovery is also specified, in which case the filesystem can
> >> +	 * be mounted with no risk of further damage.
> >> +	 */
> >> +	if (xfs_has_needsrepair(mp) && !xfs_has_norecovery(mp)) {
> > 
> > I think a better way to handle badly damaged filesystems is for us to
> > provide a means to extract directory trees in userspace, rather than
> > making the user take the risk of mounting a known-bad filesystem.
> > I've a draft of an xfs_db subcommand for doing exactly that and will
> > share for xfsprogs 6.14.
> 
> I think whether a userspace extractor is better or not depends on the
> usecase. I suppose there's some truth that a NEEDSREPAIR filesystem is
> "known bad" but we already suffer the risk of "unknown bad" filesystems
> today. (Or for that matter, the fact that we allow "norecovery" today,
> which also guarantees a mount of an inconsistent filesystem.)
> 
> "Something is wrong with this filesystem, let's mount it readonly and
> copy off the data" is a pretty time-tested approach, I think, hampered
> only by the fairly recent addition of NEEDSREPAIR.
> 
> a userspace scrape-the-device tool may well be useful for some, but
> I don't think that vs. this kernelspace option needs to be an either/or
> decision.

Fair enough; it's not like we have a tool today that can extract
directory trees from an unmountable filesystem.  Do you want to rvb this
one, then?

--D

> Thanks,
> 
> -Eric
> 
> > --D
> > 
> >>  		xfs_warn(mp, "Filesystem needs repair.  Please run xfs_repair.");
> >>  		error = -EFSCORRUPTED;
> >>  		goto out_free_sb;
> >> -- 
> >> 2.48.1
> >>
> >>
> > 
> 
> 

Re: [PATCH 1/1] xfs: do not check NEEDSREPAIR if ro,norecovery mount.

[Dave Chinner]

On Mon, Feb 03, 2025 at 09:55:13AM +0100, Lukas Herbolt wrote:
> If there is corrutpion on the filesystem andxfs_repair
> fails to repair it. The last resort of getting the data
> is to use norecovery,ro mount. But if the NEEDSREPAIR is
> set the filesystem cannot be mounted. The flag must be
> cleared out manually using xfs_db, to get access to what
> left over of the corrupted fs.
> 
> Signed-off-by: Lukas Herbolt <lukas@herbolt.com>
> ---
>  fs/xfs/xfs_super.c | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c
> index 394fdf3bb535..c2566dcc4f88 100644
> --- a/fs/xfs/xfs_super.c
> +++ b/fs/xfs/xfs_super.c
> @@ -1635,8 +1635,12 @@ xfs_fs_fill_super(
>  #endif
>  	}
>  
> -	/* Filesystem claims it needs repair, so refuse the mount. */
> -	if (xfs_has_needsrepair(mp)) {
> +	/*
> +	 * Filesystem claims it needs repair, so refuse the mount unless
> +	 * norecovery is also specified, in which case the filesystem can
> +	 * be mounted with no risk of further damage.
> +	 */
> +	if (xfs_has_needsrepair(mp) && !xfs_has_norecovery(mp)) {
>  		xfs_warn(mp, "Filesystem needs repair.  Please run xfs_repair.");
>  		error = -EFSCORRUPTED;
>  		goto out_free_sb;

Look fine. I've had to hack around this with xfs_db when testing
(broken) xfs_repair mods a couple of times myself...

Reviewed-by: Dave Chinner <dchinner@redhat.com>

-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH 1/1] xfs: do not check NEEDSREPAIR if ro,norecovery mount.

[Eric Sandeen]

On 2/3/25 2:55 AM, Lukas Herbolt wrote:
> If there is corrutpion on the filesystem andxfs_repair
> fails to repair it. The last resort of getting the data
> is to use norecovery,ro mount. But if the NEEDSREPAIR is
> set the filesystem cannot be mounted. The flag must be
> cleared out manually using xfs_db, to get access to what
> left over of the corrupted fs.
> 
> Signed-off-by: Lukas Herbolt <lukas@herbolt.com>
> ---
>  fs/xfs/xfs_super.c | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c
> index 394fdf3bb535..c2566dcc4f88 100644
> --- a/fs/xfs/xfs_super.c
> +++ b/fs/xfs/xfs_super.c
> @@ -1635,8 +1635,12 @@ xfs_fs_fill_super(
>  #endif
>  	}
>  
> -	/* Filesystem claims it needs repair, so refuse the mount. */
> -	if (xfs_has_needsrepair(mp)) {
> +	/*
> +	 * Filesystem claims it needs repair, so refuse the mount unless
> +	 * norecovery is also specified, in which case the filesystem can
> +	 * be mounted with no risk of further damage.
> +	 */
> +	if (xfs_has_needsrepair(mp) && !xfs_has_norecovery(mp)) {
>  		xfs_warn(mp, "Filesystem needs repair.  Please run xfs_repair.");
>  		error = -EFSCORRUPTED;
>  		goto out_free_sb;

thanks Lukas, this looks good to me as well. And with ro,norecovery
there should (tm) be no writes whatever to the filesystem so risk of
introducing further corruption from the mount should be zero.

Reviewed-by: Eric Sandeen <sandeen@redhat.com>

Re: [PATCH 1/1] xfs: do not check NEEDSREPAIR if ro,norecovery mount.

[Carlos Maiolino]

On Mon, 03 Feb 2025 09:55:13 +0100, Lukas Herbolt wrote:
> If there is corrutpion on the filesystem andxfs_repair
> fails to repair it. The last resort of getting the data
> is to use norecovery,ro mount. But if the NEEDSREPAIR is
> set the filesystem cannot be mounted. The flag must be
> cleared out manually using xfs_db, to get access to what
> left over of the corrupted fs.
> 
> [...]

Applied to for-next, thanks!

[1/1] xfs: do not check NEEDSREPAIR if ro,norecovery mount.
      commit: 263b984ae26bbc320581d2872a125bc305bea0b8

Best regards,
-- 
Carlos Maiolino <cem@kernel.org>