Bug 762080 (GLUSTER-348)

There are two three things to note here.

1. This segfault will not occur in release2 since inode_link works fine when we
link root into the itable.

2. Avati says that the real fix lies in changing the if(name) condition in
inode_link to if (parent). Still, we'll merge Du's fix to maintain clarity in
libglusterfsclient to show that root inode is not to be linked.

We'll also merge my fix for the NULL check:
http://patches.gluster.com/patch/2060/

and a third patch that changes inode_link, all three being merged in the
Following branches:

Du's libglusterfsclient patch -      mainline only, since this happened because
of the gen number change.

Shehjar's NULL check patch    -      release2 and mainline

inode_link fix                -      mainline only, again to fine tune the gen
number changes.

Du, is it possible for you to send the third patch also?

Thanks

PATCH: http://patches.gluster.com/patch/2076 in master (libglusterfsclient: don't do inode_link on root inode.)

(gdb) bt
#0  0xb7d44d6d in __dentry_search_arbit (inode=0x0) at ../../../libglusterfs/src/inode.c:808
#1  0xb7d44f83 in inode_path (inode=0x808e5e8, name=0x8090561 "newfilexx", bufp=0xbfad6910)
    at ../../../libglusterfs/src/inode.c:870
#2  0xb7d09110 in libgf_client_loc_fill (loc=0xbfad6964, ctx=0x8051e90, ino=0, par=1, name=0x8090561 "newfilexx")
    at ../../../libglusterfsclient/src/libglusterfsclient.c:924
#3  0xb7d1f495 in __do_path_resolve (loc=0xbfad6a28, ctx=0x8051e90, lookup_basename=1 '\001')
    at ../../../libglusterfsclient/src/libglusterfsclient-dentry.c:326
#4  0xb7d1f817 in libgf_client_path_lookup (loc=0xbfad6a28, ctx=0x8051e90, lookup_basename=1 '\001')
    at ../../../libglusterfsclient/src/libglusterfsclient-dentry.c:406
#5  0xb7d0f1ca in glusterfs_glh_open (handle=0x8051e90, path=0x8090500 "/newfilexx", flags=2369)
    at ../../../libglusterfsclient/src/libglusterfsclient.c:2797
#6  0xb7d0f701 in glusterfs_open (path=0xbfad735f "/mnt/raghu/newfilexx", flags=2369)
    at ../../../libglusterfsclient/src/libglusterfsclient.c:2926
#7  0xb7edb43a in vmp_open (pathname=0xbfad735f "/mnt/raghu/newfilexx", flags=2369)
    at ../../../booster/src/booster.c:392
#8  0xb7edb62c in booster_open (pathname=0xbfad735f "/mnt/raghu/newfilexx", use64=1, flags=2369)
    at ../../../booster/src/booster.c:455
#9  0xb7edb90f in *open64 (pathname=0xbfad735f "/mnt/raghu/newfilexx", flags=2369)
    at ../../../booster/src/booster.c:554
(gdb) l
813             }
814
815             if (!dentry) {
816                     list_for_each_entry (trav, &inode->dentry_list, inode_list) {
817                             dentry = trav;
818                             break;
819                     }
820             }
821
822             return dentry;


A dentry was added for root inode, which is incorrect. libgf_client_lookup_cbk should not call inode_link for '/'.

(In reply to comment #2)

> 
> Du, is it possible for you to send the third patch also?


Nevermind, I got it.

> 
> Thanks

PATCH: http://patches.gluster.com/patch/2081 in master (core: Check for NULL to avoid segfault)

PATCH: http://patches.gluster.com/patch/2086 in master (core: Use parent for condition to create dentry not name)

PATCH: http://patches.gluster.com/patch/2083 in release-2.0 (core: Check for NULL to avoid segfault)

(In reply to comment #2)
> There are two three things to note here.
> 
> 1. This segfault will not occur in release2 since inode_link works fine when we
> link root into the itable.

> 
> 2. Avati says that the real fix lies in changing the if(name) condition in
> inode_link to if (parent). Still, we'll merge Du's fix to maintain clarity in
> libglusterfsclient to show that root inode is not to be linked.

I thought of the same fix. But after speaking to avati, I felt it may be better to fix libglusterfsclient.

> 
> We'll also merge my fix for the NULL check:
> http://patches.gluster.com/patch/2060/
> 
> and a third patch that changes inode_link, all three being merged in the
> Following branches:
> 
> Du's libglusterfsclient patch -      mainline only, since this happened because
> of the gen number change.

This fix is not calling inode_link on root inode, right? Since it is not correct to call inode_link on root inode, should we not also fix this in release-2.0, even though there is no issue over there?

> 
> Shehjar's NULL check patch    -      release2 and mainline
> 
> inode_link fix                -      mainline only, again to fine tune the gen
> number changes.

Change if (name) to if (parent) before adding a dentry. right?

> 
> Du, is it possible for you to send the third patch also?

Yes, I'll do it.

> 
> Thanks

(In reply to comment #8)
> (In reply to comment #2)
> > There are two three things to note here.
> > 
> > 1. This segfault will not occur in release2 since inode_link works fine when we
> > link root into the itable.
> 
> > 
> > 2. Avati says that the real fix lies in changing the if(name) condition in
> > inode_link to if (parent). Still, we'll merge Du's fix to maintain clarity in
> > libglusterfsclient to show that root inode is not to be linked.
> 
> I thought of the same fix. But after speaking to avati, I felt it may be better
> to fix libglusterfsclient.
> 

I guess he changed his mind later about changing inode.c

> > 
> > We'll also merge my fix for the NULL check:
> > http://patches.gluster.com/patch/2060/
> > 
> > and a third patch that changes inode_link, all three being merged in the
> > Following branches:
> > 
> > Du's libglusterfsclient patch -      mainline only, since this happened because
> > of the gen number change.
> 
> This fix is not calling inode_link on root inode, right? Since it is not
Yes. This is what you sent.

> correct to call inode_link on root inode, should we not also fix this in
> release-2.0, even though there is no issue over there?

I dont see why we should fix it in r2. The segfault occurred only because of the
inode generation number change and that is only in mainline.

> 
> > 
> > Shehjar's NULL check patch    -      release2 and mainline
> > 
> > inode_link fix                -      mainline only, again to fine tune the gen
> > number changes.
> 
> Change if (name) to if (parent) before adding a dentry. right?
> 
> > 
> > Du, is it possible for you to send the third patch also?
> 
> Yes, I'll do it.

The patch has been submitted already. Dont worry about it.
> 
> > 
> > Thanks