[Gluster-devel] Regards to taking lock in dictionary

[Xavi Hernandez]

Hi Mohit,

On Thu, Oct 24, 2019 at 5:19 AM Mohit Agrawal <moagrawa at redhat.com> wrote:

>
> I have a query why do we take a lock at the time of doing an operation in
> a dictionary.I have observed in testing it seems there is no codepath where
>   we are using the dictionary parallel. In theory, the dictionary flow is
> like one xlator put some data in a dictionary and pass to the next xlator
> and xlator  and originator xlator won't touch dictionary until the called
> xlator returns.
>
>   To prove the same I have executed below test case
>   1) I have changed all LOCK/UNLOCK macro with dictlock/dictunlock
> function in the dictionary code and call the LOCK/UNLOCK macros
>   2) Create a 1x3 volume and mount the volume
>   3) Run stap script on one of the node to measure dict lock contention to
> log the entry if more than one thread access the dictionary at the same time
>   4) Run smallfile tool like below with multiple operations like
> create/append/cleanup
>      /root/sync.sh; python /root/smallfile/smallfile_cli.py --operation
> <op_name> --threads 8 --file-size 64 --files 5000 --top /mnt/test
>  --host-set "hp-m300-2.gsslab.pnq.redhat.com";
>
>    I have not found any single thread that is trying to access the
> dictionary while dictlock is already held by some other thread.
>
>   I have uploaded a patch(
> https://review.gluster.org/#/c/glusterfs/+/23603/) after converting the
> if condition to false in dictlock/unlock and run the
>   regression test suite.I am not getting major failures after removing the
> lock from a dictionary.
>
>   Please share your view on the same if the dictionary is not consumed by
> multiple threads at the same time still we do need to take lock
>   in the dictionary.
>   Please share if I need to test something more to validate the same.
>

That's very interesting. From a logical point of view I think that we
shouldn't have two threads accessing the same dict at the same time. The
only thing that locks guarantee is the internal integrity of the dict
structure but, if we have concurrent access to the dict, one of the threads
could see keys and/or values appearing/disappearing/changing spuriously,
which surely could make things not work well. Apparently we don't have
these issues.

If that's true, I think we should be able to completely get rid of the lock
in dict structure.

This change is dangerous however, and it would need extensive testing. If
any issue is found, probably we would need to fix that issue instead of
adding locks again though.

Another important thing to do is to run some performance tests. If we
really don't have contention in dict lock, the real cost of the lock is an
atomic operation, which is not free but it's much cheaper than the kernel
context that happens in case of contention. We would need to see the
improvement to weight the benefits of this change.

Xavi



> Regards,
> Mohit Agrawal
> _______________________________________________
>
> Community Meeting Calendar:
>
> APAC Schedule -
> Every 2nd and 4th Tuesday at 11:30 AM IST
> Bridge: https://bluejeans.com/118564314
>
> NA/EMEA Schedule -
> Every 1st and 3rd Tuesday at 01:00 PM EDT
> Bridge: https://bluejeans.com/118564314
>
> Gluster-devel mailing list
> Gluster-devel at gluster.org
> https://lists.gluster.org/mailman/listinfo/gluster-devel
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.gluster.org/pipermail/gluster-devel/attachments/20191024/d5522986/attachment.html>