From: "Darrick J. Wong" <darrick.wong@oracle.com>
To: Eric Sandeen <sandeen@sandeen.net>
Cc: sandeen@redhat.com, linux-xfs@vger.kernel.org
Subject: Re: [PATCH 24/27] xfs_scrub: fstrim the free areas if there are no errors on the filesystem
Date: Tue, 16 Jan 2018 14:23:14 -0800 [thread overview]
Message-ID: <20180116222314.GW5602@magnolia> (raw)
In-Reply-To: <36955880-90ad-86c3-9db1-59191e267533@sandeen.net>
On Tue, Jan 16, 2018 at 04:07:44PM -0600, Eric Sandeen wrote:
> On 1/5/18 7:54 PM, Darrick J. Wong wrote:
> > From: Darrick J. Wong <darrick.wong@oracle.com>
> >
> > If the filesystem scan comes out clean or fixes all the problems, call
> > fstrim to clean out the free areas (if it's an ssd/thinp/whatever).
>
> Is this the right patch header for this patch?
>
> Oh ok, this adds a "repair phase" which is really only implementing
> preen for now, which is really only fstrimming at this point.
>
> so:
>
> preen()
> if no errors
> xfs_repair_fs() (IMHO odd to call "repair" on a clean filesystem?)
> fstrim
>
> So I guess what was confusing to me is that you do "preen" work under
> "repair" functions. I get it that they might all be lumped together
> in pending work now, but I'm still wrapping my head around what does
> and doesn't happen in various modes, and how to recognize that in
> the code...
Based on our extended IRC conversations I was planning to rename the
"repair list" to "action items" so that we could have a
xfs_process_action_items() that actually takes care of issuing the
repair calls, then we could have two wrappers:
xfs_repair_fs() -> xfs_process_action_items(); fstrim();
xfs_preen_fs() -> if (!ctx->errors_found) xfs_process_action_items()
>
>
> > Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> > ---
> > scrub/Makefile | 1 +
> > scrub/phase4.c | 52 ++++++++++++++++++++++++++++++++++++++++++++++++++++
> > scrub/vfs.c | 23 +++++++++++++++++++++++
> > scrub/vfs.h | 2 ++
> > scrub/xfs_scrub.c | 26 +++++++++++++++++++++++++-
> > scrub/xfs_scrub.h | 1 +
> > 6 files changed, 104 insertions(+), 1 deletion(-)
> > create mode 100644 scrub/phase4.c
> >
> >
> > diff --git a/scrub/Makefile b/scrub/Makefile
> > index fd26624..91f99ff 100644
> > --- a/scrub/Makefile
> > +++ b/scrub/Makefile
> > @@ -41,6 +41,7 @@ inodes.c \
> > phase1.c \
> > phase2.c \
> > phase3.c \
> > +phase4.c \
> > phase5.c \
> > phase6.c \
> > phase7.c \
> > diff --git a/scrub/phase4.c b/scrub/phase4.c
> > new file mode 100644
> > index 0000000..dadf4de
> > --- /dev/null
> > +++ b/scrub/phase4.c
> > @@ -0,0 +1,52 @@
> > +/*
> > + * Copyright (C) 2018 Oracle. All Rights Reserved.
> > + *
> > + * Author: Darrick J. Wong <darrick.wong@oracle.com>
> > + *
> > + * This program is free software; you can redistribute it and/or
> > + * modify it under the terms of the GNU General Public License
> > + * as published by the Free Software Foundation; either version 2
> > + * of the License, or (at your option) any later version.
> > + *
> > + * This program is distributed in the hope that it would be useful,
> > + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> > + * GNU General Public License for more details.
> > + *
> > + * You should have received a copy of the GNU General Public License
> > + * along with this program; if not, write the Free Software Foundation,
> > + * Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301, USA.
> > + */
> > +#include <stdio.h>
> > +#include <stdint.h>
> > +#include <stdbool.h>
> > +#include <dirent.h>
> > +#include <sys/types.h>
> > +#include <sys/stat.h>
> > +#include <sys/statvfs.h>
> > +#include "xfs.h"
> > +#include "xfs_fs.h"
> > +#include "list.h"
> > +#include "path.h"
> > +#include "workqueue.h"
> > +#include "xfs_scrub.h"
> > +#include "common.h"
> > +#include "scrub.h"
> > +#include "vfs.h"
> > +
> > +/* Phase 4: Repair filesystem. */
> > +
> > +/* Fix everything that needs fixing. */
> > +bool
> > +xfs_repair_fs(
> > + struct scrub_ctx *ctx)
> > +{
> > + bool moveon = true;
> > +
> > + pthread_mutex_lock(&ctx->lock);
> > + if (moveon && ctx->errors_found == 0)
> > + fstrim(ctx);
> > + pthread_mutex_unlock(&ctx->lock);
> > +
> > + return moveon;
> > +}
> > diff --git a/scrub/vfs.c b/scrub/vfs.c
> > index 6a51090..98d356f 100644
> > --- a/scrub/vfs.c
> > +++ b/scrub/vfs.c
> > @@ -219,3 +219,26 @@ _("Could not queue directory scan work."));
> > free(sftd);
> > return false;
> > }
> > +
> > +#ifndef FITRIM
> > +struct fstrim_range {
> > + __u64 start;
> > + __u64 len;
> > + __u64 minlen;
> > +};
> > +#define FITRIM _IOWR('X', 121, struct fstrim_range) /* Trim */
> > +#endif
>
> (I wonder if we should move all these "if it ain't available define it"
> stuff into a single header file at some point...)
Yeah, probably....
> > +
> > +/* Call FITRIM to trim all the unused space in a filesystem. */
> > +void
> > +fstrim(
> > + struct scrub_ctx *ctx)
> > +{
> > + struct fstrim_range range = {0};
> > + int error;
> > +
> > + range.len = ULLONG_MAX;
> > + error = ioctl(ctx->mnt_fd, FITRIM, &range);
> > + if (error && errno != EOPNOTSUPP && errno != ENOTTY)
> > + perror(_("fstrim"));
> > +}
>
> still wondering if we should have an option to skip this, given some device's
> horrific performance under fstrim, and/or an other desire to keep an image
> whole.
I already added it in my dev tree. -k turns off FITRIM.
> > diff --git a/scrub/vfs.h b/scrub/vfs.h
> > index 100eb18..3305159 100644
> > --- a/scrub/vfs.h
> > +++ b/scrub/vfs.h
> > @@ -28,4 +28,6 @@ typedef bool (*scan_fs_tree_dirent_fn)(struct scrub_ctx *, const char *,
> > bool scan_fs_tree(struct scrub_ctx *ctx, scan_fs_tree_dir_fn dir_fn,
> > scan_fs_tree_dirent_fn dirent_fn, void *arg);
> >
> > +void fstrim(struct scrub_ctx *ctx);
> > +
> > #endif /* XFS_SCRUB_VFS_H_ */
> > diff --git a/scrub/xfs_scrub.c b/scrub/xfs_scrub.c
> > index bc40f3c..7809431 100644
> > --- a/scrub/xfs_scrub.c
> > +++ b/scrub/xfs_scrub.c
> > @@ -340,6 +340,20 @@ _("%sI/O rate: %.1f%s/s in, %.1f%s/s out, %.1f%s/s tot\n"),
> > return true;
> > }
> >
> > +/* Run the preening phase if there are no errors. */
> > +static bool
> > +preen(
> > + struct scrub_ctx *ctx)
> > +{
> > + if (ctx->errors_found) {
> > + str_info(ctx, ctx->mntpoint,
> > +_("Errors found, please re-run with -y."));
> > + return true;
> > + }
> > +
> > + return xfs_repair_fs(ctx);
> > +}
> > +
> > /* Run all the phases of the scrubber. */
> > static bool
> > run_scrub_phases(
> > @@ -393,8 +407,18 @@ run_scrub_phases(
> > /* Run all phases of the scrub tool. */
> > for (phase = 1, sp = phases; sp->fn; sp++, phase++) {
> > /* Turn on certain phases if user said to. */
> > - if (sp->fn == DATASCAN_DUMMY_FN && scrub_data)
> > + if (sp->fn == DATASCAN_DUMMY_FN && scrub_data) {
> > sp->fn = xfs_scan_blocks;
> > + } else if (sp->fn == REPAIR_DUMMY_FN) {
> > + if (ctx->mode == SCRUB_MODE_PREEN) {
> > + sp->descr = _("Preen filesystem.");
> > + sp->fn = preen;
> > + } else if (ctx->mode == SCRUB_MODE_REPAIR) {
> > + sp->descr = _("Repair filesystem.");
> > + sp->fn = xfs_repair_fs;
> > + }
> > + sp->must_run = true;
>
> if must_run is always true here, should it just be initialized in
> the structure along w/ the other must_run phases?
Ok.
--D
> > + }
> >
> > /* Skip certain phases unless they're turned on. */
> > if (sp->fn == REPAIR_DUMMY_FN ||
> > diff --git a/scrub/xfs_scrub.h b/scrub/xfs_scrub.h
> > index a5cdba8..4a383f1 100644
> > --- a/scrub/xfs_scrub.h
> > +++ b/scrub/xfs_scrub.h
> > @@ -108,5 +108,6 @@ bool xfs_scan_inodes(struct scrub_ctx *ctx);
> > bool xfs_scan_connections(struct scrub_ctx *ctx);
> > bool xfs_scan_blocks(struct scrub_ctx *ctx);
> > bool xfs_scan_summary(struct scrub_ctx *ctx);
> > +bool xfs_repair_fs(struct scrub_ctx *ctx);
> >
> > #endif /* XFS_SCRUB_XFS_SCRUB_H_ */
> >
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at http://vger.kernel.org/majordomo-info.html
> >
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
next prev parent reply other threads:[~2018-01-16 22:23 UTC|newest]
Thread overview: 61+ messages / expand[flat|nested] mbox.gz Atom feed top
2018-01-06 1:51 [PATCH v11 00/27] xfsprogs: online scrub/repair support Darrick J. Wong
2018-01-06 1:51 ` [PATCH 01/27] xfs_scrub: create online filesystem scrub program Darrick J. Wong
2018-01-12 0:16 ` Eric Sandeen
2018-01-12 1:08 ` Darrick J. Wong
2018-01-12 1:07 ` Eric Sandeen
2018-01-12 1:10 ` Darrick J. Wong
2018-01-06 1:51 ` [PATCH 02/27] xfs_scrub: common error handling Darrick J. Wong
2018-01-12 1:15 ` Eric Sandeen
2018-01-12 1:23 ` Darrick J. Wong
2018-01-06 1:51 ` [PATCH 03/27] xfs_scrub: set up command line argument parsing Darrick J. Wong
2018-01-11 23:39 ` Eric Sandeen
2018-01-12 1:53 ` Darrick J. Wong
2018-01-12 1:30 ` Eric Sandeen
2018-01-12 2:03 ` Darrick J. Wong
2018-01-06 1:51 ` [PATCH 04/27] xfs_scrub: dispatch the various phases of the scrub program Darrick J. Wong
2018-01-06 1:51 ` [PATCH 05/27] xfs_scrub: figure out how many threads we're going to need Darrick J. Wong
2018-01-06 1:52 ` [PATCH 06/27] xfs_scrub: create an abstraction for a block device Darrick J. Wong
2018-01-11 23:24 ` Eric Sandeen
2018-01-11 23:59 ` Darrick J. Wong
2018-01-12 0:04 ` Eric Sandeen
2018-01-12 1:27 ` Darrick J. Wong
2018-01-06 1:52 ` [PATCH 07/27] xfs_scrub: find XFS filesystem geometry Darrick J. Wong
2018-01-06 1:52 ` [PATCH 08/27] xfs_scrub: add inode iteration functions Darrick J. Wong
2018-01-06 1:52 ` [PATCH 09/27] xfs_scrub: add space map " Darrick J. Wong
2018-01-06 1:52 ` [PATCH 10/27] xfs_scrub: add file " Darrick J. Wong
2018-01-11 23:19 ` Eric Sandeen
2018-01-12 0:24 ` Darrick J. Wong
2018-01-06 1:52 ` [PATCH 11/27] xfs_scrub: filesystem counter collection functions Darrick J. Wong
2018-01-06 1:52 ` [PATCH 12/27] xfs_scrub: wrap the scrub ioctl Darrick J. Wong
2018-01-11 23:12 ` Eric Sandeen
2018-01-12 0:28 ` Darrick J. Wong
2018-01-06 1:52 ` [PATCH 13/27] xfs_scrub: scan filesystem and AG metadata Darrick J. Wong
2018-01-06 1:52 ` [PATCH 14/27] xfs_scrub: thread-safe stats counter Darrick J. Wong
2018-01-06 1:53 ` [PATCH 15/27] xfs_scrub: scan inodes Darrick J. Wong
2018-01-06 1:53 ` [PATCH 16/27] xfs_scrub: check directory connectivity Darrick J. Wong
2018-01-06 1:53 ` [PATCH 17/27] xfs_scrub: warn about suspicious characters in directory/xattr names Darrick J. Wong
2018-01-06 1:53 ` [PATCH 18/27] xfs_scrub: warn about normalized Unicode name collisions Darrick J. Wong
2018-01-16 23:52 ` Eric Sandeen
2018-01-16 23:57 ` Eric Sandeen
2018-01-16 23:59 ` Darrick J. Wong
2018-01-06 1:53 ` [PATCH 19/27] xfs_scrub: create a bitmap data structure Darrick J. Wong
2018-01-06 1:53 ` [PATCH 20/27] xfs_scrub: create infrastructure to read verify data blocks Darrick J. Wong
2018-01-06 1:53 ` [PATCH 21/27] xfs_scrub: scrub file " Darrick J. Wong
2018-01-11 23:25 ` Eric Sandeen
2018-01-12 0:29 ` Darrick J. Wong
2018-01-06 1:53 ` [PATCH 22/27] xfs_scrub: optionally use SCSI READ VERIFY commands to scrub data blocks on disk Darrick J. Wong
2018-01-06 1:53 ` [PATCH 23/27] xfs_scrub: check summary counters Darrick J. Wong
2018-01-06 1:54 ` [PATCH 24/27] xfs_scrub: fstrim the free areas if there are no errors on the filesystem Darrick J. Wong
2018-01-16 22:07 ` Eric Sandeen
2018-01-16 22:23 ` Darrick J. Wong [this message]
2018-01-06 1:54 ` [PATCH 25/27] xfs_scrub: progress indicator Darrick J. Wong
2018-01-11 23:27 ` Eric Sandeen
2018-01-12 0:32 ` Darrick J. Wong
2018-01-06 1:54 ` [PATCH 26/27] xfs_scrub: create a script to scrub all xfs filesystems Darrick J. Wong
2018-01-06 1:54 ` [PATCH 27/27] xfs_scrub: integrate services with systemd Darrick J. Wong
2018-01-06 3:50 ` [PATCH 07/27] xfs_scrub: find XFS filesystem geometry Darrick J. Wong
2018-01-12 4:17 ` [PATCH v11 00/27] xfsprogs: online scrub/repair support Eric Sandeen
2018-01-17 1:31 ` Darrick J. Wong
2018-01-16 19:21 ` [PATCH 28/27] xfs_scrub: wire up repair ioctl Darrick J. Wong
2018-01-16 19:21 ` [PATCH 29/27] xfs_scrub: schedule and manage repairs to the filesystem Darrick J. Wong
-- strict thread matches above, loose matches on Subject: below --
2017-11-17 21:00 [PATCH v10 00/27] xfsprogs-4.15: online scrub/repair support Darrick J. Wong
2017-11-17 21:02 ` [PATCH 24/27] xfs_scrub: fstrim the free areas if there are no errors on the filesystem Darrick J. Wong
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20180116222314.GW5602@magnolia \
--to=darrick.wong@oracle.com \
--cc=linux-xfs@vger.kernel.org \
--cc=sandeen@redhat.com \
--cc=sandeen@sandeen.net \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).