[PATCH 8/9] Enable generic filesystems to be fsck'd
Eric Sandeen
sandeen at sandeen.net
Tue Jun 2 10:10:44 CDT 2009
Christoph Hellwig wrote:
> On Thu, May 28, 2009 at 08:51:28AM -0400, Christoph Hellwig wrote:
>> On Wed, May 27, 2009 at 01:53:32PM -0500, Eric Sandeen wrote:
>>> This includes a fair bit of rearranging to avoid code duplication,
>>> but the goal is to allow 'fsck -n -t $FSTYP $device' to be run on
>>> any generic filesystem.
>>>
>>> Any FS for which this doesn't work will need it's own fsck routine.
>> Looks generally good, some comments:
>>
>> - I would get rid of _check_generic_test_fs and just opencode the
>> _check_generic_filesystem $TEST_DEV in the two callers.
>> - why the odd calling convention of _is_mounted which allows to
>> optionally pass the fstype? Currently we only have one caller
>> that doesn't pass it, and if we grow one that needs it I would
>> rather always pass it explicitly..
>>
>> Btw, I seems like _check_testdir is never actually called, and I can't
>> really see a reason why it would be different from _check_test_fs.
>
> Here's a version with those changes and additionally making sure
> _check_test_fs continues to be a no-op for nfs and udf.
Thanks :) nitpicky comments below, mostly probably nitpicking stuff
that was in my original patch ;)
> Index: xfstests-dev/common.rc
> ===================================================================
> --- xfstests-dev.orig/common.rc 2009-06-02 12:12:24.000000000 +0000
> +++ xfstests-dev/common.rc 2009-06-02 12:21:36.000000000 +0000
> @@ -707,29 +707,29 @@
> [ "$?" == "0" ] || _notrun "$qa_user user not defined."
> }
>
> -# check that a FS is mounted as XFS. if so, return mount point
> +# check that a FS on a device is mounted
> +# if so, return mount point
> #
> -_xfs_mounted()
> +_is_mounted()
> {
> if [ $# -ne 1 ]
> then
> - echo "Usage: _xfs_mounted device" 1>&2
> + echo "Usage: _is_mounted device" 1>&2
> exit 1
> fi
>
> device=$1
>
> - if _mount | grep "$device " | $AWK_PROG '
> - /type xfs/ { print $3 ; exit 0 }
> - END { exit 1 }
> + if _mount | grep "$device " | $AWK_PROG -v pattern="type $FSTYP" '
> + pattern { print $3 ; exit 0 }
> + END { exit 1 }
> '
> then
> - echo "_xfs_mounted: $device is not a mounted XFS FS"
> + echo "_is_mounted: $device is not a mounted $FSTYP FS"
> exit 1
> fi
> }
>
> -
> # remount a FS to a new mode (ro or rw)
> #
> _remount()
> @@ -749,14 +749,105 @@
> fi
> }
>
> -# run xfs_check and friends on a FS.
> +# Run the apropriate repair/check on a filesystem
appropriate (that was probably my typo to start with!)
> #
> # if the filesystem is mounted, it's either remounted ro before being
> # checked or it's unmounted and then remounted
> #
>
> +# If set, we remount ro instead of unmounting for fsck
> USE_REMOUNT=0
>
> +_umount_or_remount_ro()
> +{
> + if [ $# -ne 1 ]
> + then
> + echo "Usage: _umount_or_remount_ro device" 1>&2
<device> might be clearer
> + exit 1
> + fi
> + device=$1
> +
> + if [ $USE_REMOUNT -eq 0 ]
> + then
> + mountpoint=`_is_mounted $device`
> + $UMOUNT_PROG $device
> + else
> + _remount $device ro
> + fi
> + echo "$mountpoint"
Maybe we should move the mountpoint assignment outside the conditional,
since we echo it unconditionally. Only the !USE_REMOUNT case cares
anyway but still...
> +}
> +
> +_mount_or_remount_rw()
> +{
> + if [ $# -ne 3 ]
> + then
> + echo "Usage: _mount_or_remount_rw opts device mountpoint" 1>&2
<opts> <device> <mountpoint>
> + exit 1
> + fi
> + mount_opts=$1
> + device=$2
> + mountpoint=$3
> +
> + if [ $USE_REMOUNT -eq 0 ]
> + then
> + if ! _mount -t $FSTYP $mount_opts $device $mountpoint
> + then
> + echo "!!! failed to remount $device on $mountpoint"
> + return 0 # ok=0
> + fi
> + else
> + _remount $device rw
> + fi
> +
> + return 1 # ok=1
> +}
# Check a generic filesystem in no-op mode; this assumes that the
# underlying fsck program accepts "-n" for a no-op (check-only) run,
# and that it will still return an errno for corruption in this mode.
#
# Filesystems which don't support this will need to define their
# own check routine.
> +_check_generic_filesystem()
> +{
> + device=$1
> +
> + # If type is set, we're mounted
> + type=`_fs_type $device`
> + ok=1
> +
> + if [ "$type" = "$FSTYP" ]
> + then
> + # mounted ...
> + mountpoint=`_umount_or_remount_ro $device`
> + fi
> +
> + fsck -t $FSTYP -n $device >$tmp.fsck 2>&1
> + if [ $? -ne 0 ]
> + then
> + echo "_check_generic_filesystem: filesystem on $device is inconsistent (see $seq.full)"
> +
> + echo "_check_generic filesystem: filesystem on $device is inconsistent" >>$here/$seq.full
> + echo "*** fsck.$FSTYP output ***" >>$here/$seq.full
> + cat $tmp.fsck >>$here/$seq.full
> + echo "*** end fsck.$FSTYP output" >>$here/$seq.full
> +
> + ok=0
> + fi
> + rm -f $tmp.fsck
> +
> + if [ $ok -eq 0 ]
> + then
> + echo "*** mount output ***" >>$here/$seq.full
> + _mount >>$here/$seq.full
> + echo "*** end mount output" >>$here/$seq.full
> + elif [ "$type" = "$FSTYP" ]
> + then
> + # was mounted ...
> + _mount_or_remount_rw "$MOUNT_OPTIONS" $device $mountpoint
oops tab vs. space here
> + ok=$?
> + fi
> +
> + [ $ok -eq 0 ] && exit 1
> + return 0
> +}
> +
> +# run xfs_check and friends on a FS.
> +
> _check_xfs_filesystem()
> {
> if [ $# -ne 3 ]
> @@ -787,15 +878,8 @@
>
> if [ "$type" = "xfs" ]
> then
> - # mounted...
> -
> - if [ $USE_REMOUNT -eq 0 ]
> - then
> - mountpoint=`_xfs_mounted $device`
> - $UMOUNT_PROG $device
> - else
> - _remount $device ro
> - fi
> + # mounted ...
> + mountpoint=`_umount_or_remount_ro $device`
> fi
>
> $XFS_LOGPRINT_PROG -t $extra_log_options $device 2>&1 \
> @@ -848,17 +932,7 @@
> echo "*** end mount output" >>$here/$seq.full
> elif [ "$type" = "xfs" ]
> then
> - # mounted...
> - if [ $USE_REMOUNT -eq 0 ]
> - then
> - if ! _mount -t xfs $extra_mount_options $device $mountpoint
> - then
> - echo "!!! failed to remount $device on $mountpoint"
> - ok=0
> - fi
> - else
> - _remount $device rw
> - fi
> + _mount_or_remount_rw "$extra_mount_options" $device $mountpoint
> fi
>
> [ $ok -eq 0 ] && exit 1
> @@ -908,12 +982,8 @@
>
> }
>
> -_check_test_fs()
> +_check_xfs_test_fs()
> {
> - if [ "$FSTYP" != "xfs" ]; then
> - return
> - fi
> -
> TEST_LOG="none"
> TEST_RT="none"
> [ "$USE_EXTERNAL" = yes -a ! -z "$TEST_LOGDEV" ] && \
> @@ -932,6 +1002,24 @@
> fi
> }
>
> +_check_test_fs()
> +{
> + case $FSTYP in
> + xfs)
> + _check_xfs_test_fs
> + ;;
> + nfs)
> + # no way to check consistency for nfs
> + ;;
> + udf)
> + # do nothing for now
> + ;;
> + *)
> + _check_generic_filesystem $TEST_DEV
fix indentation here ...
> + ;;
> + esac
> +}
> +
> _check_scratch_fs()
> {
> case $FSTYP in
> @@ -953,6 +1041,7 @@
> # Don't know how to check an NFS filesystem, yet.
> ;;
> *)
> + _check_generic_filesystem $SCRATCH_DEV
> ;;
> esac
> }
> @@ -987,25 +1076,6 @@
> echo "$os/$platform $host $kernel"
> }
>
> -_check_testdir()
> -{
> - case $FSTYP in
> - xfs)
> - _check_test_fs
> - ;;
> - udf)
> - _cleanup_testdir
> - _check_scratch_fs
> - _scratch_mount
> - ;;
> - nfs*)
> - # Don't know how to check an NFS filesystem, yet.
> - ;;
> - *)
> - ;;
> - esac
> -}
> -
> _setup_udf_scratchdir()
> {
> [ "$FSTYP" != "udf" ] \
>
More information about the xfs
mailing list