[Gluster-devel] Possible problem introduced by http://review.gluster.org/15573

Fri Oct 21 14:15:06 UTC 2016

On 10/21/2016 06:35 PM, Soumya Koduri wrote:
> Hi Xavi,
>
> On 10/21/2016 12:57 PM, Xavier Hernandez wrote:
>> Looking at the code, I think that the added fd_unref() should only be
>> called if the fop preparation fails. Otherwise the callback already
>> unreferences the fd.
>>
>> Code flow:
>>
>> * glfs_fsync_async_common() takes an fd ref and calls STACK_WIND passing
>> that fd.
>> * Just after that a ref is released.
>> * When glfs_io_async_cbk() is called another ref is released.
>>
>> Note that if fop preparation fails, a single fd_unref() is called, but
>> on success two fd_unref() are called.
>
> Sorry for the inconvenience caused. I think its patch#15573 hasn't
> caused the problem but has highlighted another ref leak in the code.
>
> From the code I see that glfs_io_async_cbk() does fd_unref (glfd->fd)
> but not the fd passed in STACK_WIND_COOKIE() of the fop.
>
> If I take any fop, for eg.,
> glfs_fsync_common() {
>
>        fd = glfs_resolve_fd (glfd->fs, subvol, glfd);
>
>
> }
>
> Here in glfs_resolve_fd ()
>
> fd_t *
> __glfs_resolve_fd (struct glfs *fs, xlator_t *subvol, struct glfs_fd *glfd)
> {
>         fd_t *fd = NULL;
>
>         if (glfd->fd->inode->table->xl == subvol)
>                 return fd_ref (glfd->fd);
>
>>>>>>>>> Here we can see that  we are taking extra ref additional to the
> ref already taken for glfd->fd. That means the caller of this function
> needs to fd_unref(fd) irrespective of subsequent fd_unref (glfd->fd).
>
>         fd = __glfs_migrate_fd (fs, subvol, glfd);
>         if (!fd)
>                 return NULL;
>
>
>         if (subvol == fs->active_subvol) {
>                 fd_unref (glfd->fd);
>                 glfd->fd = fd_ref (fd);
>         }
>
>>>>>>>>> I think the issue is here during graph_switch(). You have
> mentioned as well that the crash happens post graph_switch. Maybe here
> we are missing an extra ref to be taken for fd additional to glfd->fd. I
> need to look through __glfs_migrate_fd() to confirm that. But these are
> my initial thoughts.

Looking into this, I think we should fix glfs_io_async_cbk() not to 
fd_unref(glfd->fd). glfd->fd should be active though out the lifetime of 
glfd (i.e, until it is closed). Thoughts?
Also could you please check if it is the second/subsequent fsync_async() 
call which results in crash?

Thanks,
Soumya

>
> Please let me know your comments.
>
> Thanks,
> Soumya
>
>
>>
>> Xavi
>>
>> On 21/10/16 09:03, Xavier Hernandez wrote:
>>> Hi,
>>>
>>> I've just tried Gluster 3.8.5 with Proxmox using gfapi and I
>>> consistently see a crash each time an attempt to connect to the volume
>>> is made.
>>>
>>> The backtrace of the crash shows this:
>>>
>>> #0  pthread_spin_lock () at
>>> ../nptl/sysdeps/x86_64/pthread_spin_lock.S:24
>>> #1  0x00007fe5345776a5 in fd_unref (fd=0x7fe523f7205c) at fd.c:553
>>> #2  0x00007fe53482ba18 in glfs_io_async_cbk (op_ret=<optimized out>,
>>> op_errno=0, frame=<optimized out>, cookie=0x7fe526c67040,
>>> iovec=iovec at entry=0x0, count=count at entry=0)
>>>     at glfs-fops.c:839
>>> #3  0x00007fe53482beed in glfs_fsync_async_cbk (frame=<optimized out>,
>>> cookie=<optimized out>, this=<optimized out>, op_ret=<optimized out>,
>>> op_errno=<optimized out>,
>>>     prebuf=<optimized out>, postbuf=0x7fe5217fe890, xdata=0x0) at
>>> glfs-fops.c:1382
>>> #4  0x00007fe520be2eb7 in ?? () from
>>> /usr/lib/x86_64-linux-gnu/glusterfs/3.8.5/xlator/debug/io-stats.so
>>> #5  0x00007fe5345d118a in default_fsync_cbk (frame=0x7fe52ceef3ac,
>>> cookie=0x560ef95398e8, this=0x8, op_ret=0, op_errno=0, prebuf=0x1,
>>> postbuf=0x7fe5217fe890, xdata=0x0) at defaults.c:1508
>>> #6  0x00007fe5345d118a in default_fsync_cbk (frame=0x7fe52ceef204,
>>> cookie=0x560ef95398e8, this=0x8, op_ret=0, op_errno=0, prebuf=0x1,
>>> postbuf=0x7fe5217fe890, xdata=0x0) at defaults.c:1508
>>> #7  0x00007fe525f78219 in dht_fsync_cbk (frame=0x7fe52ceef2d8,
>>> cookie=0x560ef95398e8, this=0x0, op_ret=0, op_errno=0,
>>> prebuf=0x7fe5217fe820, postbuf=0x7fe5217fe890, xdata=0x0)
>>>     at dht-inode-read.c:873
>>> #8  0x00007fe5261bbc7f in client3_3_fsync_cbk (req=0x7fe525f78030
>>> <dht_fsync_cbk>, iov=0x7fe526c61040, count=8, myframe=0x7fe52ceef130) at
>>> client-rpc-fops.c:975
>>> #9  0x00007fe5343201f0 in rpc_clnt_handle_reply (clnt=0x18,
>>> clnt at entry=0x7fe526fafac0, pollin=0x7fe526c3a1c0) at rpc-clnt.c:791
>>> #10 0x00007fe53432056c in rpc_clnt_notify (trans=<optimized out>,
>>> mydata=0x7fe526fafaf0, event=<optimized out>, data=0x7fe526c3a1c0) at
>>> rpc-clnt.c:962
>>> #11 0x00007fe53431c8a3 in rpc_transport_notify (this=<optimized out>,
>>> event=<optimized out>, data=<optimized out>) at rpc-transport.c:541
>>> #12 0x00007fe5283e8d96 in socket_event_poll_in (this=0x7fe526c69440) at
>>> socket.c:2267
>>> #13 0x00007fe5283eaf37 in socket_event_handler (fd=<optimized out>,
>>> idx=5, data=0x7fe526c69440, poll_in=1, poll_out=0, poll_err=0) at
>>> socket.c:2397
>>> #14 0x00007fe5345ab3f6 in event_dispatch_epoll_handler
>>> (event=0x7fe5217fecc0, event_pool=0x7fe526ca2040) at event-epoll.c:571
>>> #15 event_dispatch_epoll_worker (data=0x7fe527c0f0c0) at
>>> event-epoll.c:674
>>> #16 0x00007fe5324140a4 in start_thread (arg=0x7fe5217ff700) at
>>> pthread_create.c:309
>>> #17 0x00007fe53214962d in clone () at
>>> ../sysdeps/unix/sysv/linux/x86_64/clone.S:111
>>>
>>> The fd being unreferenced contains this:
>>>
>>> (gdb) print *fd
>>> $6 = {
>>>   pid = 97649,
>>>   flags = 2,
>>>   refcount = 0,
>>>   inode_list = {
>>>     next = 0x7fe523f7206c,
>>>     prev = 0x7fe523f7206c
>>>   },
>>>   inode = 0x0,
>>>   lock = {
>>>     spinlock = 1,
>>>     mutex = {
>>>       __data = {
>>>         __lock = 1,
>>>         __count = 0,
>>>         __owner = 0,
>>>         __nusers = 0,
>>>         __kind = 0,
>>>         __spins = 0,
>>>         __elision = 0,
>>>         __list = {
>>>           __prev = 0x0,
>>>           __next = 0x0
>>>         }
>>>       },
>>>       __size = "\001", '\000' <repeats 38 times>,
>>>       __align = 1
>>>     }
>>>   },
>>>   _ctx = 0x7fe52ec31c40,
>>>   xl_count = 11,
>>>   lk_ctx = 0x7fe526c126a0,
>>>   anonymous = _gf_false
>>> }
>>>
>>> fd->inode is NULL, explaining the cause of the crash. We also see that
>>> fd->refcount is already 0. So I'm wondering if this couldn't be an extra
>>> fd_unref() introduced by that patch.
>>>
>>> The crash seems to happen immediately after a graph switch.
>>>
>>> Xavi
>>> _______________________________________________
>>> Gluster-devel mailing list
>>> Gluster-devel at gluster.org
>>> http://www.gluster.org/mailman/listinfo/gluster-devel
> _______________________________________________
> Gluster-devel mailing list
> Gluster-devel at gluster.org
> http://www.gluster.org/mailman/listinfo/gluster-devel