Hi Dan,
On Fri, 23 Dec 2011 10:00:54 -0500, Dan McGee wrote:
> On Fri, Dec 23, 2011 at 12:56 AM, Ryusuke Konishi
> <konishi.ryusuke@xxxxxxxxxxxxx> wrote:
> > On Wed, 21 Dec 2011 15:34:09 -0600, Dan McGee wrote:
> >> When running as non-root, the error message seems to indicate a
> >> checkpoint is a snapshot, rather than the actual problem, which is that
> >> I do not have permission to delete the checkpoints.
> >>
> >> Before:
> >> $ rmcp 29..35
> >> rmcp: 29: cannot remove snapshot
> >> rmcp: 30: cannot remove snapshot
> >> rmcp: 31: cannot remove snapshot
> >> rmcp: 32: cannot remove snapshot
> >> rmcp: 33: cannot remove snapshot
> >> rmcp: 34: cannot remove snapshot
> >> rmcp: 35: cannot remove snapshot
> >>
> >> To delete snapshot(s), change them into checkpoints with
> >> chcp command before removal.
> >>
> >> After:
> >> $ ./bin/rmcp 29..35
> >> lt-rmcp: 29: cannot remove checkpoint: Operation not permitted
> >> lt-rmcp: 30: cannot remove checkpoint: Operation not permitted
> >> lt-rmcp: 31: cannot remove checkpoint: Operation not permitted
> >> lt-rmcp: 32: cannot remove checkpoint: Operation not permitted
> >> lt-rmcp: 33: cannot remove checkpoint: Operation not permitted
> >> lt-rmcp: 34: cannot remove checkpoint: Operation not permitted
> >> lt-rmcp: 35: cannot remove checkpoint: Operation not permitted
> >>
> >> Since May 2009 the kernel has returned EBUSY instead of EPERM for
> >> removal requests against snapshots, commit 30c25be71fcbd87fd. We should
> >> be able to treat EPERM as expected now, as the commit message there
> >> indicates.
> >>
> >> Signed-off-by: Dan McGee <dan@xxxxxxxxxxxxx>
> >> ---
> >>
> >> This does introduce a behavior change as well- we no longer exit the range loop
> >> immediately on an error code, instead we continue the loop and attempt to
> >> delete the rest of the checkpoints in the range. Objections to this? I'm not an
> >> expert by any means.
> >>
> >> bin/rmcp.c | 10 ++++++----
> >> 1 files changed, 6 insertions(+), 4 deletions(-)
> >>
> >> diff --git a/bin/rmcp.c b/bin/rmcp.c
> >> index 780a192..3c5c184 100644
> >> --- a/bin/rmcp.c
> >> +++ b/bin/rmcp.c
> >> @@ -105,7 +105,7 @@ static int rmcp_remove_range(struct nilfs *nilfs,
> >> nd++;
> >> continue;
> >> }
> >> - if (errno == EBUSY || errno == EPERM) {
> >> + if (errno == EBUSY) {
> >
> > OK, let's stop regarding the EPERM error as EBUSY. For most kernels,
> > it's more important to distinguish real permission errors from the
> > snapshot removal error.
> >
> > We have only recently bumped the version of nilfs-utils to 2.1.x from
> > 2.0.x. It's a good opportunity for this change.
> >
> >> nss++;
> >> if (!force) {
> >> fprintf(stderr,
> >> @@ -115,14 +115,16 @@ static int rmcp_remove_range(struct nilfs *nilfs,
> >> } else if (errno == ENOENT) {
> >> nocp++;
> >> } else {
> >> - fprintf(stderr, "%s: %s\n", progname, strerror(errno));
> >> + fprintf(stderr,
> >> + "%s: %llu: cannot remove checkpoint: %s\n",
> >> + progname, (unsigned long long)cno,
> >> + strerror(errno));
> >> ret = -1;
> >
> >> - goto out;
> >
> > Could you revise this ?
> >
> > I think the loop should abort immidiately when the rmcp meets an
> > important error even though it will change behavior for the real
> > permission error.
>
> 1. Please define "important"- is EPERM important? I would tend to say
> the error should be printed once per checkpoint as it is now in this
> case for EPERM, and then restore the prior behavior for other actions.
I meant something critical (e.g. ENOMEM, EIO) or violative (EPERM).
The original EPERM check was just a workaround for early nilfs
implementation, which returns EPERM when trying to remove checkpoints
in snapshot mode.
Now EPERM is returned only if the caller is not allowed to
remove/change checkpoints.
If the caller does not have proper right, every remove operation fails
and it never succeeds partially. Although the result is predictable,
it may repeat thousands of the same error messages.
As for the critical errors, I think continuing operations is not
undesirable not to worsen the situation.
> 2. I'm not very fond of the error messages with the immediate exit:
>
> $ ./bin/rmcp 100..105 115..125
> lt-rmcp: 100: cannot remove checkpoint: Operation not permitted
> $ echo $?
> 1
>
> Did 101-105 succeed? Did the second range succeed? Judging from this
> (lack of) output, I would think it did, which is quite obviously not
> the case.
Right. It's ambiguous from the current messages.
How about printing an additional message to indicate the range of
unprocessed checkpoints if aborting the loop ?
104: cannot remove checkpoint: xxxxx
Checkpoints 104..105 115..125 were not removed.
With regards,
Ryusuke Konishi
> So I would vote to not exit early in at least the semi-normal EPERM
> case; perhaps we do for the others. What is the reasoning behind
> exiting early in even the abnormal error cases?
>
> >> }
> >> }
> >> if (!force && (nss > 0 || (nocp > 0 && nd == 0)))
> >> ret = 1;
> >> - out:
> >> +
> >> *ndeleted = nd;
> >> *nsnapshots = nss;
> >> return ret;
> >> --
> --
> To unsubscribe from this list: send the line "unsubscribe linux-nilfs" in
> the body of a message to majordomo@xxxxxxxxxxxxxxx
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-nilfs" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
