Hi,
>>> On Thu, 16 Oct 2014 13:53:05 +0000
in message "Re: [lxc-devel] [PATCH RFC] overlayfs: overlayfs.v22 or
higher needs workdir option"
Serge Hallyn-san wrote:
> Hi,
> Mostly ok, just a few cmments below. If you like I'll make the
> changes as I apply, or you can send a new version.
Thanks for your comment! I have some questions.
> > This patch is
> > * the workdir is LXCPATH/ct/olwork.
> > * mkdir workdir when clone
> > * mount without workdir is failed, then retry mount with workdir
> >
> > I've tested on:
> > * 3.14.4 with overlayfs.v21 (not need workdir=) (priv and unpriv)
> > * 3.17.0 with overlayfs.v24 (need workdir=) (priv and unpriv)
> >
> > Signed-off-by: KATOH Yasufumi <ka...@jazz.email.ne.jp>
> > ---
> > src/lxc/bdev.c | 77
> > ++++++++++++++++++++++++++++++++++++++++++++++++++++------
> > 1 file changed, 70 insertions(+), 7 deletions(-)
> >
> > diff --git a/src/lxc/bdev.c b/src/lxc/bdev.c
> > index 4fc10f2..69c2e7f 100644
> > --- a/src/lxc/bdev.c
> > +++ b/src/lxc/bdev.c
> > @@ -2154,10 +2154,11 @@ static int overlayfs_detect(const char *path)
> > static int overlayfs_mount(struct bdev *bdev)
> > {
> > char *options, *dup, *lower, *upper;
> > - int len;
> > + char *options_work, *work, *worktmp;
> > + int len, len2;
> > unsigned long mntflags;
> > char *mntdata;
> > - int ret;
> > + int ret, ret2;
> >
> > if (strcmp(bdev->type, "overlayfs"))
> > return -22;
> > @@ -2175,6 +2176,14 @@ static int overlayfs_mount(struct bdev *bdev)
> > *upper = '\0';
> > upper++;
> >
> > + // workdir option that is needed overlayfs.v22 or higher
> > + // workdir=$LXCDIR/ct/olwork
> > + work = alloca(strlen(upper)+1);
> > + strcpy(work, upper);
> > + if (!(worktmp = rindex(work, '/')))
> > + return -22;
> > + strncpy(worktmp+1, "olwork", 6);
> can't do this exactly, because olwork my be longer than the upper
> filename specified by the container. Just liberally adding 6 to
> the size you alloca should be ok.
> (because upper does not have to be 'rootfs')
I assume that only lxc-clone create 'upper' dir. So I think that
'upper' dir is always "/var/lib/lxc/CT/delta0". So I replace "delta0"
to "olwork" (same 6 length ^^;).
Are there any cases that upper dir is not "/var/lib/lxc/CT/delta0"?
(But we think about creating overlayfs container manually not using
lxc-create?)
> > +
> > if (parse_mntopts(bdev->mntopts, &mntflags, &mntdata) < 0) {
> > free(mntdata);
> > return -22;
> > @@ -2187,21 +2196,42 @@ static int overlayfs_mount(struct bdev *bdev)
> > len = strlen(lower) + strlen(upper) +
> > strlen("upperdir=,lowerdir=,") + strlen(mntdata) + 1;
> > options = alloca(len);
> > ret = snprintf(options, len, "upperdir=%s,lowerdir=%s,%s",
> > upper, lower, mntdata);
> > +
> > + len2 = strlen(lower) + strlen(upper) + strlen(work)
> > + + strlen("upperdir=,lowerdir=,workdir=") +
> > strlen(mntdata) + 1;
> > + options_work = alloca(len2);
> > + ret2 = snprintf(options, len2,
> > "upperdir=%s,lowerdir=%s,workdir=%s,%s",
> > + upper, lower, work, mntdata);
> > }
> > else {
> > len = strlen(lower) + strlen(upper) +
> > strlen("upperdir=,lowerdir=") + 1;
> > options = alloca(len);
> > ret = snprintf(options, len, "upperdir=%s,lowerdir=%s", upper,
> > lower);
> > +
> > + len2 = strlen(lower) + strlen(upper) + strlen(work)
> > + + strlen("upperdir=,lowerdir=,workdir=") + 1;
> > + options_work = alloca(len2);
> > + ret2 = snprintf(options_work, len2,
> > "upperdir=%s,lowerdir=%s,workdir=%s",
> > + upper, lower, work);
> > }
> > - if (ret < 0 || ret >= len) {
> > + if (ret < 0 || ret >= len || ret2 < 0 || ret2 >= len2) {
> > free(mntdata);
> > return -1;
> > }
> >
> > ret = mount(lower, bdev->dest, "overlayfs", MS_MGC_VAL | mntflags,
> > options);
> > - if (ret < 0)
> > - SYSERROR("overlayfs: error mounting %s onto %s options %s",
> > + if (ret < 0) {
> > + WARN("overlayfs: error mounting %s onto %s options %s",
> > lower, bdev->dest, options);
> Let's actually make this an INFO. Or actually it'll be clear from the INFO
> below
> so we can just drop the message altogether.
OK!
> > + // retry with workdir option
> > + ret = mount(lower, bdev->dest, "overlayfs", MS_MGC_VAL |
> > mntflags, options_work);
> > + if (ret < 0)
> > + SYSERROR("overlayfs: error mounting %s onto %s options
> > %s",
> > + lower, bdev->dest, options_work);
> > + else
> > + INFO("overlayfs: mounted %s onto %s options %s",
> > + lower, bdev->dest, options_work);
> > + }
> > else
> > INFO("overlayfs: mounted %s onto %s options %s",
> > lower, bdev->dest, options);
> > @@ -2265,7 +2295,7 @@ static int overlayfs_clonepaths(struct bdev *orig,
> > struct bdev *new, const char
> > WARN("Failed to update ownership of %s", new->dest);
> >
> > if (strcmp(orig->type, "dir") == 0) {
> > - char *delta;
> > + char *delta, *work;
> > int ret, len;
> >
> > // if we have /var/lib/lxc/c2/rootfs, then delta will be
> > @@ -2287,6 +2317,23 @@ static int overlayfs_clonepaths(struct bdev *orig,
> > struct bdev *new, const char
> > if (am_unpriv() && chown_mapped_root(delta, conf) < 0)
> > WARN("Failed to update ownership of %s", delta);
> >
> > + // mkdir workdir that is needed on overlayfs.v22 or higher
> > + // if we have /var/lib/lxc/c2/rootfs, then workdir will be
> > + // /var/lib/lxc/c2/olwork
> > + work = strdup(new->dest);
> I realize code above this is doing the same thing , and it actually
> works out because we always create the new container with bdev->dest NULL
> and then in bdev_copy fill it in ending in rootfs. But I'd prefer we
> change that in both places so that if/when that flow changes we dont
> get mysterious occasional segvs.
Oh! This part is mistake! (^_^;) I intended to do
work = strdup(delta);
same as above.
> > + if (!work)
> > + return -1;
> > +
> > + strcpy(&work[strlen(work)-6], "olwork");
> > + if (mkdir(work, 0755) < 0) {
> > + SYSERROR("error: mkdir %s", work);
> > + free(work);
> > + return -1;
> > + }
> > + if (am_unpriv() && chown_mapped_root(work, conf) < 0)
> > + WARN("Failed to update ownership of %s", work);
> > + free(work);
> > +
> > // the src will be 'overlayfs:lowerdir:upperdir'
> > len = strlen(delta) + strlen(orig->src) + 12;
> > new->src = malloc(len);
> > @@ -2303,7 +2350,7 @@ static int overlayfs_clonepaths(struct bdev *orig,
> > struct bdev *new, const char
> > // I think we want to use the original lowerdir, with a
> > // private delta which is originally rsynced from the
> > // original delta
> > - char *osrc, *odelta, *nsrc, *ndelta;
> > + char *osrc, *odelta, *nsrc, *ndelta, *work;
> > int len, ret;
> > if (!(osrc = strdup(orig->src)))
> > return -22;
> > @@ -2327,6 +2374,22 @@ static int overlayfs_clonepaths(struct bdev *orig,
> > struct bdev *new, const char
> > }
> > if (am_unpriv() && chown_mapped_root(ndelta, conf) < 0)
> > WARN("Failed to update ownership of %s", ndelta);
> > +
> > + // mkdir workdir for overlayfs.v22 or later
> Workdir temporary holds files before atomically switching with
> the destination, so it's ok for it to be empty. Let's a comment
> to that effect so future reviewers can feel sure about that
> without looking at the overlayfs details.
OK!
> > + work = strdup(ndelta);
> > + if (!work)
> > + return -1;
> > +
> > + strcpy(&work[strlen(work)-6], "olwork");
> > + if (mkdir(work, 0755) < 0) {
> > + SYSERROR("error: mkdir %s", work);
> > + free(work);
> > + return -1;
> > + }
> > + if (am_unpriv() && chown_mapped_root(work, conf) < 0)
> > + WARN("Failed to update ownership of %s", work);
> > + free(work);
> > +
> > struct rsync_data_char rdata;
> > rdata.src = odelta;
> > rdata.dest = ndelta;
> > --
> > 2.1.1
> >
> > _______________________________________________
> > lxc-devel mailing list
> > lxc-devel@lists.linuxcontainers.org
> > http://lists.linuxcontainers.org/listinfo/lxc-devel
> _______________________________________________
> lxc-devel mailing list
> lxc-devel@lists.linuxcontainers.org
> http://lists.linuxcontainers.org/listinfo/lxc-devel
_______________________________________________
lxc-devel mailing list
lxc-devel@lists.linuxcontainers.org
http://lists.linuxcontainers.org/listinfo/lxc-devel
