[Gluster-devel] Bitrot stub forget()
Kotresh Hiremath Ravishankar
khiremat at redhat.com
Thu Feb 18 04:41:18 UTC 2016
I will take care of putting up the patch upstream.
Thanks and Regards,
Kotresh H R
----- Original Message -----
> From: "Venky Shankar" <vshankar at redhat.com>
> To: "FNU Raghavendra Manjunath" <rabhat at redhat.com>
> Cc: "Gluster Devel" <gluster-devel at gluster.org>, "Kotresh Hiremath Ravishankar" <khiremat at redhat.com>
> Sent: Thursday, February 18, 2016 9:44:27 AM
> Subject: Re: Bitrot stub forget()
>
> On Tue, Feb 16, 2016 at 11:38:24PM -0500, FNU Raghavendra Manjunath wrote:
> > Venky,
> >
> > Yes. You are right. We should not remove the quarantine entry in forget.
> >
> > We have to remove it upon getting -ve lookups in bit-rot-stub and upon
> > getting an unlink.
> >
> > I have attached a patch for it.
> >
> > Unfortunately rfc.sh is failing for me with the below error.
>
> Thanks for the patch. One of us will take care of putting it up for review.
>
> >
> >
> > ssh: connect to host git.gluster.com port 22: Connection timed out
> > fatal: Could not read from remote repository.
> >
> > Please make sure you have the correct access rights
> > and the repository exists."
> >
> >
> > Regards,
> > Raghavendra
> >
> >
> > On Tue, Feb 16, 2016 at 10:53 AM, Venky Shankar <vshankar at redhat.com>
> > wrote:
> >
> > > Hey Raghu,
> > >
> > > Bitrot stub inode forget implementation (br_stub_forget()) deletes the
> > > bad
> > > object
> > > marker (under quarantine directory) if present. This looks incorrect as
> > > ->forget()
> > > can be trigerred when inode table LRU size exceeeds configured limit -
> > > check bug
> > > #1308961 which tracks this issue. I recall that protocol/server calls
> > > inode_forget()
> > > on negative lookup (that might not invoke ->forget() though) and that's
> > > the reason
> > > why br_stub_forget() has this code.
> > >
> > > So, would it make sense to purge bad object marker just in lookup()?
> > > There
> > > might be
> > > a need to do the same in unlink() in case the object was removed by the
> > > client.
> > >
> > > Thoughts?
> > >
> > > Thanks,
> > >
> > > Venky
> > >
>
> > From a0cc49172df24e263e0db25c53b57f58c19d2cab Mon Sep 17 00:00:00 2001
> > From: Raghavendra Bhat <raghavendra at redhat.com>
> > Date: Tue, 16 Feb 2016 20:22:36 -0500
> > Subject: [PATCH] features/bitrot: do not remove the quarantine handle in
> > forget
> >
> > If an object is marked as bad, then an entry is corresponding to the
> > bad object is created in the .glusterfs/quarantine directory to help
> > scrub status. The entry name is the gfid of the corrupted object.
> > The quarantine handle is removed in below 2 cases.
> >
> > 1) When protocol/server revceives the -ve lookup on an entry whose inode
> > is there in the inode table (it can happen when the corrupted object
> > is deleted directly from the backend for recovery purpose) it sends a
> > forget on the inode and bit-rot-stub removes the quarantine handle in
> > upon getting the forget.
> > refer to the below commit
> > f853ed9c61bf65cb39f859470a8ffe8973818868:
> > http://review.gluster.org/12743)
> >
> > 2) When bit-rot-stub itself realizes that lookup on a corrupted object
> > has failed with ENOENT.
> >
> > But with step1, there is a problem when the bit-rot-stub receives forget
> > due to lru limit exceeding in the inode table. In such cases, though the
> > corrupted object is not deleted (either from the mount point or from the
> > backend), the handle in the quarantine directory is removed and that object
> > is not shown in the bad objects list in the scrub status command.
> >
> > So it is better to follow only 2nd step (i.e. bit-rot-stub removing the
> > handle
> > from the quarantine directory in -ve lookups). Also the handle has to be
> > removed
> > when a corrupted object is unlinked from the mount point itself.
> >
> > Change-Id: Ibc3bbaf4bc8a5f8986085e87b729ab912cbf8cf9
> > Signed-off-by: Raghavendra Bhat <raghavendra at redhat.com>
> > ---
> > xlators/features/bit-rot/src/stub/bit-rot-stub.c | 103
> > +++++++++++++++++++++--
> > 1 file changed, 95 insertions(+), 8 deletions(-)
> >
> > diff --git a/xlators/features/bit-rot/src/stub/bit-rot-stub.c
> > b/xlators/features/bit-rot/src/stub/bit-rot-stub.c
> > index 9c0f622..920aa7c 100644
> > --- a/xlators/features/bit-rot/src/stub/bit-rot-stub.c
> > +++ b/xlators/features/bit-rot/src/stub/bit-rot-stub.c
> > @@ -2841,19 +2841,46 @@ unwind:
> >
> > /** {{{ */
> >
> > -/* forget() */
> > +/* unlink() */
> >
> > int
> > -br_stub_forget (xlator_t *this, inode_t *inode)
> > +br_stub_unlink_cbk (call_frame_t *frame, void *cookie, xlator_t *this,
> > + int32_t op_ret, int32_t op_errno, struct iatt
> > *preparent,
> > + struct iatt *postparent, dict_t *xdata)
> > {
> > - uint64_t ctx_addr = 0;
> > - br_stub_inode_ctx_t *ctx = NULL;
> > + br_stub_local_t *local = NULL;
> > + inode_t *inode = NULL;
> > + uint64_t ctx_addr = 0;
> > + br_stub_inode_ctx_t *ctx = NULL;
> > + int32_t ret = -1;
> >
> > - inode_ctx_del (inode, this, &ctx_addr);
> > - if (!ctx_addr)
> > - return 0;
> > + if (op_ret < 0)
> > + goto unwind;
> >
> > - ctx = (br_stub_inode_ctx_t *) (long) ctx_addr;
> > + local = frame->local;
> > + frame->local = NULL;
> > +
> > + inode = local->u.context.inode;
> > +
> > + ret = br_stub_get_inode_ctx (this, inode, &ctx_addr);
> > + if (ret) {
> > + /**
> > + * If the inode is bad AND context is not there, then
> > there
> > + * is a possibility of the gfid of the object being listed
> > + * in the quarantine directory and will be shown in the
> > + * bad objects list. So continuing with the fop with a
> > + * warning log. The entry from the quarantine directory
> > + * has to be removed manually. Its not a good idea to fail
> > + * the fop, as the object has already been deleted.
> > + */
> > + gf_msg (this->name, GF_LOG_WARNING, 0,
> > + BRS_MSG_GET_INODE_CONTEXT_FAILED,
> > + "failed to get the context for the inode %s",
> > + uuid_utoa (inode->gfid));
> > + goto unwind;
> > + }
> > +
> > + ctx = (br_stub_inode_ctx_t *)(long)ctx_addr;
> >
> > LOCK (&inode->lock);
> > {
> > @@ -2868,6 +2895,65 @@ br_stub_forget (xlator_t *this, inode_t *inode)
> > }
> > UNLOCK (&inode->lock);
> >
> > +unwind:
> > + STACK_UNWIND_STRICT (unlink, frame, op_ret, op_errno, preparent,
> > postparent, xdata);
> > + br_stub_cleanup_local (local);
> > + br_stub_dealloc_local (local);
> > + return 0;
> > +}
> > +
> > +int
> > +br_stub_unlink (call_frame_t *frame, xlator_t *this, loc_t *loc, int flag,
> > + dict_t *xdata)
> > +{
> > + br_stub_local_t *local = NULL;
> > + int32_t op_ret = -1;
> > + int32_t op_errno = 0;
> > +
> > + local = br_stub_alloc_local (this);
> > + if (!local) {
> > + op_ret = -1;
> > + op_errno = ENOMEM;
> > + gf_msg (this->name, GF_LOG_ERROR, ENOMEM,
> > BRS_MSG_NO_MEMORY,
> > + "failed to allocate memory for local (path: %s,
> > gfid: %s)",
> > + loc->path, uuid_utoa (loc->inode->gfid));
> > + goto unwind;
> > + }
> > +
> > + br_stub_fill_local (local, NULL, NULL, loc->inode,
> > + loc->inode->gfid,
> > + BR_STUB_NO_VERSIONING, 0);
> > +
> > + frame->local = local;
> > +
> > + STACK_WIND (frame, br_stub_unlink_cbk, FIRST_CHILD (this),
> > + FIRST_CHILD (this)->fops->unlink, loc, flag, xdata);
> > + return 0;
> > +
> > +unwind:
> > + STACK_UNWIND_STRICT (unlink, frame, op_ret, op_errno, NULL, NULL,
> > NULL);
> > + return 0;
> > +}
> > +
> > +
> > +/** }}} */
> > +
> > +/** {{{ */
> > +
> > +/* forget() */
> > +
> > +int
> > +br_stub_forget (xlator_t *this, inode_t *inode)
> > +{
> > + uint64_t ctx_addr = 0;
> > + br_stub_inode_ctx_t *ctx = NULL;
> > +
> > + inode_ctx_del (inode, this, &ctx_addr);
> > + if (!ctx_addr)
> > + return 0;
> > +
> > + ctx = (br_stub_inode_ctx_t *) (long) ctx_addr;
> > +
> > GF_FREE (ctx);
> >
> > return 0;
> > @@ -3133,6 +3219,7 @@ struct xlator_fops fops = {
> > .setxattr = br_stub_setxattr,
> > .opendir = br_stub_opendir,
> > .readdir = br_stub_readdir,
> > + .unlink = br_stub_unlink,
> > };
> >
> > struct xlator_cbks cbks = {
> > --
> > 2.5.0
> >
>
>
More information about the Gluster-devel
mailing list