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

All of lore.kernel.org
 help / color / mirror / Atom feed

* [PATCH 0/1] RFE: Do not check NEEDSREPAIR if ro,norecovery mount.
@ 2025-02-03  8:55 Lukas Herbolt
  2025-02-03  8:55 ` [PATCH 1/1] xfs: do " Lukas Herbolt
  0 siblings, 1 reply; 8+ messages in thread
From: Lukas Herbolt @ 2025-02-03  8:55 UTC (permalink / raw)
  To: cem, djwong; +Cc: linux-xfs, 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

^ permalink raw reply	[flat|nested] 8+ messages in thread

* [PATCH 1/1] xfs: do not check NEEDSREPAIR if ro,norecovery mount.
  2025-02-03  8:55 [PATCH 0/1] RFE: Do not check NEEDSREPAIR if ro,norecovery mount Lukas Herbolt
@ 2025-02-03  8:55 ` Lukas Herbolt
  2025-02-03 22:26   ` Darrick J. Wong
                     ` (3 more replies)
  0 siblings, 4 replies; 8+ messages in thread
From: Lukas Herbolt @ 2025-02-03  8:55 UTC (permalink / raw)
  To: cem, djwong; +Cc: linux-xfs, 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


^ permalink raw reply related	[flat|nested] 8+ messages in thread

* Re: [PATCH 1/1] xfs: do not check NEEDSREPAIR if ro,norecovery mount.
  2025-02-03  8:55 ` [PATCH 1/1] xfs: do " Lukas Herbolt
@ 2025-02-03 22:26   ` Darrick J. Wong
  2025-02-04 17:55     ` Eric Sandeen
  2025-02-04 20:18   ` Dave Chinner
                     ` (2 subsequent siblings)
  3 siblings, 1 reply; 8+ messages in thread
From: Darrick J. Wong @ 2025-02-03 22:26 UTC (permalink / raw)
  To: Lukas Herbolt; +Cc: cem, linux-xfs

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
> 
> 

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH 1/1] xfs: do not check NEEDSREPAIR if ro,norecovery mount.
  2025-02-03 22:26   ` Darrick J. Wong
@ 2025-02-04 17:55     ` Eric Sandeen
  2025-02-04 19:59       ` Darrick J. Wong
  0 siblings, 1 reply; 8+ messages in thread
From: Eric Sandeen @ 2025-02-04 17:55 UTC (permalink / raw)
  To: Darrick J. Wong, Lukas Herbolt; +Cc: cem, linux-xfs

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
>>
>>
> 


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH 1/1] xfs: do not check NEEDSREPAIR if ro,norecovery mount.
  2025-02-04 17:55     ` Eric Sandeen
@ 2025-02-04 19:59       ` Darrick J. Wong
  0 siblings, 0 replies; 8+ messages in thread
From: Darrick J. Wong @ 2025-02-04 19:59 UTC (permalink / raw)
  To: Eric Sandeen; +Cc: Lukas Herbolt, cem, linux-xfs

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
> >>
> >>
> > 
> 
> 

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH 1/1] xfs: do not check NEEDSREPAIR if ro,norecovery mount.
  2025-02-03  8:55 ` [PATCH 1/1] xfs: do " Lukas Herbolt
  2025-02-03 22:26   ` Darrick J. Wong
@ 2025-02-04 20:18   ` Dave Chinner
  2025-02-04 20:37   ` Eric Sandeen
  2025-02-11  8:44   ` Carlos Maiolino
  3 siblings, 0 replies; 8+ messages in thread
From: Dave Chinner @ 2025-02-04 20:18 UTC (permalink / raw)
  To: Lukas Herbolt; +Cc: cem, djwong, linux-xfs

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

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH 1/1] xfs: do not check NEEDSREPAIR if ro,norecovery mount.
  2025-02-03  8:55 ` [PATCH 1/1] xfs: do " Lukas Herbolt
  2025-02-03 22:26   ` Darrick J. Wong
  2025-02-04 20:18   ` Dave Chinner
@ 2025-02-04 20:37   ` Eric Sandeen
  2025-02-11  8:44   ` Carlos Maiolino
  3 siblings, 0 replies; 8+ messages in thread
From: Eric Sandeen @ 2025-02-04 20:37 UTC (permalink / raw)
  To: Lukas Herbolt, cem, djwong; +Cc: linux-xfs

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>

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH 1/1] xfs: do not check NEEDSREPAIR if ro,norecovery mount.
  2025-02-03  8:55 ` [PATCH 1/1] xfs: do " Lukas Herbolt
                     ` (2 preceding siblings ...)
  2025-02-04 20:37   ` Eric Sandeen
@ 2025-02-11  8:44   ` Carlos Maiolino
  3 siblings, 0 replies; 8+ messages in thread
From: Carlos Maiolino @ 2025-02-11  8:44 UTC (permalink / raw)
  To: djwong, Lukas Herbolt; +Cc: linux-xfs

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>


^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, other threads:[~2025-02-11  8:44 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-02-03  8:55 [PATCH 0/1] RFE: Do not check NEEDSREPAIR if ro,norecovery mount Lukas Herbolt
2025-02-03  8:55 ` [PATCH 1/1] xfs: do " Lukas Herbolt
2025-02-03 22:26   ` Darrick J. Wong
2025-02-04 17:55     ` Eric Sandeen
2025-02-04 19:59       ` Darrick J. Wong
2025-02-04 20:18   ` Dave Chinner
2025-02-04 20:37   ` Eric Sandeen
2025-02-11  8:44   ` Carlos Maiolino

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.