Hi,
On Wed, 02 Dec 2009 18:20:10 +0100, David Smid <[email protected]> wrote:
> Ryusuke Konishi napsal(a):
> > On Wed, 02 Dec 2009 00:44:22 +0900 (JST), Ryusuke Konishi
> > <[email protected]> wrote:
> >> Hi,
> >> On Sat, 28 Nov 2009 22:16:31 +0100, David Smid <[email protected]>
> >> wrote:
> >>> Thanks you for your valuable comments.
> >>>
> >>> You are right, of course. No kernel code changes are needed.
> >>> Here is my take #2.
> >>>
> >>> David
> >> Thank you, David. I reviewed the patch in detail, and no worrisome
> >> point found.
> >>
> >> During tests, I noticed that remount doesn't work for this option:
> >>
> >> For instance, after doing
> >> # mount -t nilfs2 -o pp=600 /dev/sdb1 /test
> >> # mount -t nilfs2 -o remount,pp=400 /dev/sdb1 /test
> >
> > Ryusuke
>
> Good point, remount should definitely work because it is quite useful to
> change
> pp on the fly.
>
> This patch should work.
>
> David
Thank you for revising the patch. I reviewed the code anew.
I will comment inline.
> --- sbin/mount/mount.nilfs2.h.mount_pp_opt 2009-07-19 16:53:00.000000000
> +0200
> +++ sbin/mount/mount.nilfs2.h 2009-12-02 10:27:26.000000000 +0100
> @@ -13,12 +13,13 @@
> #define NILFS2_FS_NAME "nilfs2"
> #define CLEANERD_NAME "nilfs_cleanerd"
> #define PIDOPT_NAME "gcpid"
> +#define PPOPT_NAME "pp"
>
> #define CLEANERD_WAIT_RETRY_COUNT 3
> #define CLEANERD_WAIT_RETRY_INTERVAL 2 /* in seconds */
>
>
> -extern int start_cleanerd(const char *, const char *, pid_t *);
> +extern int start_cleanerd(const char *, const char *, unsigned long, pid_t
> *);
> extern int stop_cleanerd(const char *, pid_t);
> extern int check_cleanerd(const char *, pid_t);
>
> --- sbin/mount/cleaner_ctl.c.mount_pp_opt 2009-07-19 16:53:00.000000000
> +0200
> +++ sbin/mount/cleaner_ctl.c 2009-12-02 18:16:16.000000000 +0100
> @@ -45,6 +45,7 @@
>
> const char cleanerd[] = "/sbin/" CLEANERD_NAME;
> const char cleanerd_nofork_opt[] = "-n";
> +const char cleanerd_protperiod_opt[] = "-p";
>
> extern char *progname;
>
> @@ -54,12 +55,14 @@
> return (kill(pid, 0) == 0);
> }
>
> -int start_cleanerd(const char *device, const char *mntdir, pid_t *ppid)
> +int start_cleanerd(const char *device, const char *mntdir,
> + unsigned long protperiod, pid_t *ppid)
> {
> - const char *dargs[5];
> + const char *dargs[7];
> struct stat statbuf;
> int i = 0;
> int res;
> + char buf[256];
>
> if (stat(cleanerd, &statbuf) != 0) {
> error(_("Warning: %s not found"), CLEANERD_NAME);
> @@ -80,6 +83,11 @@
> }
> dargs[i++] = cleanerd;
> dargs[i++] = cleanerd_nofork_opt;
> + if (protperiod != ULONG_MAX) {
> + dargs[i++] = cleanerd_protperiod_opt;
> + snprintf(buf, sizeof(buf), "%lu", protperiod);
> + dargs[i++] = buf;
> + }
> dargs[i++] = device;
> dargs[i++] = mntdir;
> dargs[i] = NULL;
> --- sbin/mount/umount.nilfs2.c.mount_pp_opt 2009-07-19 16:53:00.000000000
> +0200
> +++ sbin/mount/umount.nilfs2.c 2009-12-02 18:15:37.000000000 +0100
> @@ -69,6 +69,9 @@
> const char gcpid_opt_fmt[] = PIDOPT_NAME "=%d";
> typedef int gcpid_opt_t;
>
> +const char pp_opt_fmt[] = PPOPT_NAME "=%lu";
> +typedef unsigned long pp_opt_t;
> +
> struct umount_options {
> int flags;
> int force;
> @@ -317,6 +320,7 @@
> int res, alive = 0;
> const char *loopdev;
> pid_t pid;
> + pp_opt_t prot_period;
>
> if (streq (node, "/") || streq (node, "root"))
> nomtab++;
> @@ -349,7 +353,11 @@
> progname, spec);
> }
> } else if (alive && !check_cleanerd(spec, pid)) {
> - if (start_cleanerd(spec, node, &pid) == 0) {
> + if (find_opt(mc->m.mnt_opts, pp_opt_fmt, &prot_period)
> + < 0)
> + prot_period = ULONG_MAX;
> +
> + if (start_cleanerd(spec, node, prot_period, &pid) == 0)
> {
> if (verbose)
> printf(_("%s: restarted %s(pid=%d)\n"),
> progname, CLEANERD_NAME,
> --- sbin/mount/mount.nilfs2.c.mount_pp_opt 2009-07-19 16:53:00.000000000
> +0200
> +++ sbin/mount/mount.nilfs2.c 2009-12-02 18:14:50.000000000 +0100
> @@ -71,6 +71,9 @@
> const char gcpid_opt_fmt[] = PIDOPT_NAME "=%d";
> typedef int gcpid_opt_t;
>
> +const char pp_opt_fmt[] = PPOPT_NAME "=%lu";
> +typedef unsigned long pp_opt_t;
> +
> struct mount_options {
> char *fstype;
> char *opts;
> @@ -225,16 +228,43 @@
> *opts = newopts;
> }
>
> -static char *fix_extra_opts_string(const char *extra_opts, pid_t gcpid)
> +static void update_pp_opt(char **opts, pp_opt_t protection_period)
> +{
> + char buf[256], *newopts;
> + pp_opt_t oldpp;
> +
> + *buf = 0;
> + if (protection_period != ULONG_MAX) {
> + snprintf(buf, sizeof(buf), pp_opt_fmt, protection_period);
> + }
In case of the kernel code, these braces (i.e '{' and '}') are removed
because it's a single statement. (for your information. This is not
important.)
> + newopts = change_opt(*opts, pp_opt_fmt, &oldpp, buf);
> + my_free(*opts);
> + *opts = newopts;
> +}
> +
> +static char *fix_extra_opts_string(const char *extra_opts, pid_t gcpid,
> + pp_opt_t protection_period)
> {
> char buf[256];
> gcpid_opt_t id;
> + pp_opt_t pp;
> + char *tmpres;
> + char *res;
>
> buf[0] = '\0';
> if (gcpid)
> snprintf(buf, sizeof(buf), gcpid_opt_fmt, (int)gcpid);
> /* The gcpid option will be removed if gcpid == 0 */
> - return change_opt(extra_opts, gcpid_opt_fmt, &id, buf);
> + tmpres = change_opt(extra_opts, gcpid_opt_fmt, &id, buf);
> +
> + buf[0] = '\0';
> + if (protection_period != ULONG_MAX)
> + snprintf(buf, sizeof(buf), pp_opt_fmt, protection_period);
> + /* The pp option will be removed if pp == ULONG_MAX */
> + res = change_opt(tmpres, pp_opt_fmt, &pp, buf);
> +
> + my_free(tmpres);
> + return res;
> }
>
> /*
> @@ -312,6 +342,7 @@
> pid_t gcpid;
> int type;
> int mounted;
> + pp_opt_t protperiod;
> };
>
> static int check_mtab(void)
> @@ -348,6 +379,7 @@
> {
> struct mntentchn *mc;
> gcpid_opt_t pid;
> + pp_opt_t prot_period;
> int res = -1;
>
> if (!(mo->flags & MS_REMOUNT) && mounted(NULL, mi->mntdir)) {
> @@ -359,6 +391,9 @@
> mi->gcpid = 0;
> mi->optstr = NULL;
> mi->mounted = mounted(mi->device, mi->mntdir);
> + mi->protperiod = ULONG_MAX;
> + if (find_opt(mo->extra_opts, pp_opt_fmt, &prot_period) >= 0)
> + mi->protperiod = prot_period;
These two lines should be removed because prepare_mount() is the
function to find an existing mount and extract its options from mtab.
mo->extra_opts has new mount options.
Please see the code I add below.
> if (mo->flags & MS_BIND)
> return 0;
> @@ -375,15 +410,13 @@
> goto failed;
> case MS_RDONLY: /* ro-mount (a rw-mount exists) */
> break;
> - case MS_REMOUNT: /* rw->rw remount */
> - if (check_remount_dir(mc, mi->mntdir) < 0)
> - goto failed;
> - if (find_opt(mc->m.mnt_opts, gcpid_opt_fmt, &pid) >= 0)
> - mi->gcpid = pid;
> - mi->optstr = xstrdup(mc->m.mnt_opts); /* previous opts */
> - mi->type = RW2RW_REMOUNT;
> - break;
> - case MS_RDONLY | MS_REMOUNT: /* rw->ro remount */
> + case MS_REMOUNT | MS_RDONLY: /* rw->ro remount */
> + mi->type = RW2RO_REMOUNT;
> + mi->protperiod = ULONG_MAX;
> + /* fallthrough */
> + case MS_REMOUNT:
> + if (!(mo->flags & MS_RDONLY))
> + mi->type = RW2RW_REMOUNT; /* rw->rw remount */
>
Trailing space chars. should be removed.
> if (check_remount_dir(mc, mi->mntdir) < 0)
> goto failed;
Here, can get the existing pp value:
prot_period = ULONG_MAX;
if (find_opt(mc->m.mnt_opts, pp_opt_fmt, &prot_period) >= 0)
mi->protperiod = prot_period;
This previous value is used in do_mount_one(), for example, when
remount fails and it restarts cleanerd.
> pid = 0;
> @@ -395,7 +428,6 @@
> }
> mi->gcpid = pid;
> mi->optstr = xstrdup(mc->m.mnt_opts); /* previous opts */
> - mi->type = RW2RO_REMOUNT;
> break;
> }
>
> @@ -408,9 +440,13 @@
> do_mount_one(struct nilfs_mount_info *mi, const struct mount_options *mo)
> {
> int res, errsv;
> + char *exopts;
> + pp_opt_t prot_period;
> +
> + exopts = change_opt(mo->extra_opts, pp_opt_fmt, &prot_period, "");
>
> res = mount(mi->device, mi->mntdir, fstype, mo->flags & ~MS_NOSYS,
> - mo->extra_opts);
> + exopts);
> if (!res)
> goto out;
>
> @@ -425,15 +461,19 @@
> progname, mi->device, mi->mntdir, strerror(errsv));
> break;
> }
> - if (mi->type != RW2RO_REMOUNT)
> + if (mi->type != RW2RO_REMOUNT && mi->type != RW2RW_REMOUNT)
> goto out;
> + /* Cleaner daemon was stopped and it needs to run */
> + /* because filesystem is still mounted */
> if (check_mtab()) {
> /* Restarting cleaner daemon */
> - if (start_cleanerd(mi->device, mi->mntdir, &mi->gcpid) == 0) {
> + if (start_cleanerd(mi->device, mi->mntdir, mi->protperiod,
> + &mi->gcpid) == 0) {
> if (verbose)
> printf(_("%s: restarted %s\n"),
> progname, CLEANERD_NAME);
> update_gcpid_opt(&mi->optstr, mi->gcpid);
> + update_pp_opt(&mi->optstr, mi->protperiod);
This update_pp_opt() is redundant because this is an error path and
the pp option should keep the previous value. On the other hand,
update_gcpid_opt() is necessary since GC pid is always changed after
the restart.
> update_mtab_entry(mi->device, mi->mntdir, fstype,
> mi->optstr, 0, 0, !mi->mounted);
> } else {
> @@ -443,31 +483,39 @@
> } else
> printf(_("%s not restarted\n"), CLEANERD_NAME);
> out:
> + my_free(exopts);
> return res;
> }
>
> static void update_mount_state(struct nilfs_mount_info *mi,
> const struct mount_options *mo)
> {
> - pid_t pid = (mi->type == RW2RW_REMOUNT) ? mi->gcpid : 0;
> + pid_t pid = fake ? mi->gcpid : 0;
Right. The old gcpid will break except for the fake mode.
> char *exopts;
> int rungc;
>
> + if (mo->flags & MS_RDONLY)
> + mi->protperiod = ULONG_MAX;
> +
New pp option can be parsed here.
if ((mo->flags & MS_RDONLY) || (mo->flags & MS_BIND) ||
find_opt(mo->extra_opts, pp_opt_fmt, &prot_period) < 0)
mi->protperiod = ULONG_MAX;
else
mi->protperiod = prot_period;
Note that we don't invoke cleanerd for a bind mount.
> rungc = !(mo->flags & MS_RDONLY) && !(mo->flags & MS_BIND) &&
> - (mi->type != RW2RW_REMOUNT || mi->gcpid == 0);
> + (pid == 0);
> if (!check_mtab()) {
> if (rungc)
> printf(_("%s not started\n"), CLEANERD_NAME);
> return;
> }
> if (rungc) {
> - if (start_cleanerd(mi->device, mi->mntdir, &pid) < 0)
> + if (start_cleanerd(mi->device, mi->mntdir, mi->protperiod,
> + &pid) < 0)
> error(_("%s aborted"), CLEANERD_NAME);
> else if (verbose)
> printf(_("%s: started %s\n"), progname, CLEANERD_NAME);
> - }
> + } else
> + if (!fake)
> + pid = 0;
> +
> my_free(mi->optstr);
> - exopts = fix_extra_opts_string(mo->extra_opts, pid);
> + exopts = fix_extra_opts_string(mo->extra_opts, pid, mi->protperiod);
> mi->optstr = fix_opts_string(((mo->flags & ~MS_NOMTAB) | MS_NETDEV),
> exopts, NULL);
>
> --- man/mount.nilfs2.8.mount_pp_opt 2009-07-19 16:53:00.000000000 +0200
> +++ man/mount.nilfs2.8 2009-12-02 10:27:26.000000000 +0100
> @@ -103,6 +103,12 @@
> remount the file system read-only, or panic and halt the system.) The
> default is continue.
> .TP
> +.BR pp=\fP\fIprotection-period\fP
> +Specify the \fIprotection-period\fP for the cleaner daemon (in
> +seconds). nilfs_cleanerd never deletes recent checkpoints whose
> +elapsed time from its creation is smaller than
> +\fIprotection-period\fP.
> +.TP
> .BR order=relaxed " / " order=strict
> Specify order semantics for file data. Metadata is always written to
> follow the POSIX semantics about the order of filesystem operations.
Thanks,
Ryusuke Konishi
_______________________________________________
users mailing list
[email protected]
https://www.nilfs.org/mailman/listinfo/users
