Re: Bitrot stub forget()

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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@xxxxxxxxxx> 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@xxxxxxxxxx>
> 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@xxxxxxxxxx>
> ---
>  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
> 

_______________________________________________
Gluster-devel mailing list
Gluster-devel@xxxxxxxxxxx
http://www.gluster.org/mailman/listinfo/gluster-devel



[Index of Archives]     [Gluster Users]     [Ceph Users]     [Linux ARM Kernel]     [Linux ARM]     [Linux Omap]     [Fedora ARM]     [IETF Annouce]     [Security]     [Bugtraq]     [Linux]     [Linux OMAP]     [Linux MIPS]     [eCos]     [Asterisk Internet PBX]     [Linux API]

  Powered by Linux