Updated the glusterd_volinfo_find() to use hash table

[nik-redhat]

Added the code to use urcu hash table storing volinfo and updated the glusterd_volinfo_find() to use hashing for finding the volumes.

This PR references issue #1615

Signed-off-by: nik-redhat nladha@redhat.com

[mohit84]

/run regression

[mohit84]

/run brick-mux regression

[nik-redhat]

/run regression

[nik-redhat]

/run brick-mux regression

[nik-redhat]

/run regression

[nik-redhat]

/run regression

[nik-redhat]

/recheck smoke

[nik-redhat]

One question... why we don't simply use:

return strcmp((const char *)_key, node->volname) == 0;

First of all, in gf_strncpy() we need to pass the size of the target buffer key, not the length of the source string _key. Additionally, the copy is not useful. Why we need to copy it if what we will do is a simple comparison ?

Didn't think that way. This looks better, will make the update.

But, here we should check with strncmp i.e, using the length of _key too, isn't it?

[nik-redhat]

/run regression

[nik-redhat]

/run regression

[xhernandez]

One question... why we don't simply use:

return strcmp((const char *)_key, node->volname) == 0;

First of all, in gf_strncpy() we need to pass the size of the target buffer key, not the length of the source string _key. Additionally, the copy is not useful. Why we need to copy it if what we will do is a simple comparison ?

Didn't think that way. This looks better, will make the update.

But, here we should check with strncmp i.e, using the length of _key too, isn't it?

Why, if we don't use an intermediate buffer, we don't have any length limitation here.

[nik-redhat]

@xhernandez to be honest I am not sure about the LOCKS, I took the reference from the Github repo for the urcu hash implementation and made the changes accordingly with my level of understanding. I may be wrong in that, so I am adding the link for the same here to give you a reference, if you still think it should be updated I will do it that way :+1: Article links: article1 article2 Github links: Addition of data , Deletion of data, Lookup

[xhernandez]

@xhernandez to be honest I am not sure about the LOCKS, I took the reference from the Github repo for the urcu hash implementation and made the changes accordingly with my level of understanding. I may be wrong in that, so I am adding the link for the same here to give you a reference, if you still think it should be updated I will do it that way Article links: article1 article2 Github links: Addition of data , Deletion of data, Lookup

I didn't check if this implementation supported concurrent updates (my bad). It seems this is already handled by the hash, so a read lock is enough for this case.

However, what is still a problem is that once you leave the read-lock section, the element retrieved by a lookup could be destroyed by another thread. If we want to keep using it outside the read-lock section, we need to use a reference counting mechanism or copy it (i.e. put some mark that prevents the entry from being deleted once removed from the hash table if it's still in use).

[xhernandez]

However, what is still a problem is that once you leave the read-lock section, the element retrieved by a lookup could be destroyed by another thread. If we want to keep using it outside the read-lock section, we need to use a reference counting mechanism or copy it (i.e. put some mark that prevents the entry from being deleted once removed from the hash table if it's still in use).

Or extend the read-lock section to cover the whole usage of the returned value.

[nik-redhat]

However, what is still a problem is that once you leave the read-lock section, the element retrieved by a lookup could be destroyed by another thread. If we want to keep using it outside the read-lock section, we need to use a reference counting mechanism or copy it (i.e. put some mark that prevents the entry from being deleted once removed from the hash table if it's still in use).

The node obtained is not required after the read-lock section, as I am saving the volinfo member of the node to the structure volinfo before exiting the read-lock section which is on line:1810 *volinfo = tmp_volinfo_node->volinfo;. So, I think it won't cause any issue.

[xhernandez]

However, what is still a problem is that once you leave the read-lock section, the element retrieved by a lookup could be destroyed by another thread. If we want to keep using it outside the read-lock section, we need to use a reference counting mechanism or copy it (i.e. put some mark that prevents the entry from being deleted once removed from the hash table if it's still in use).

The node obtained is not required after the read-lock section, as I am saving the volinfo member of the node to the structure volinfo before exiting the read-lock section which is on line:1810 *volinfo = tmp_volinfo_node->volinfo;. So, I think it won't cause any issue.

When a glusterd_volinfo_node structure is deleted after removing it from the hash table, the associated volinfo is also destroyed (line 15322). So it cannot be used outside the read-lock section.

[nik-redhat]

When a glusterd_volinfo_node structure is deleted after removing it from the hash table, the associated volinfo is also destroyed (line 15322). So it cannot be used outside the read-lock section.

Understood. We are not using the volinfo outside the read-lock section, associated with the node once that node is deleted.

[xhernandez]

When a glusterd_volinfo_node structure is deleted after removing it from the hash table, the associated volinfo is also destroyed (line 15322). So it cannot be used outside the read-lock section.

Understood. We are not using the volinfo outside the read-lock section, associated with the node once that node is deleted.

You are using the volinfo outside the read-lock section. In glusterd_volinfo_find() you search the volume inside the read-lock section, but once you find it, the read-lock section terminates and the volinfo is returned. This means that callers of glusterd_volinfo_find() can be accessing a destroyed element if another thread has removed the volinfo from the list.

[nik-redhat]

You are using the volinfo outside the read-lock section. In glusterd_volinfo_find() you search the volume inside the read-lock section, but once you find it, the read-lock section terminates and the volinfo is returned. This means that callers of glusterd_volinfo_find() can be accessing a destroyed element if another thread has removed the volinfo from the list.

Okay, I get your point. But I have one question like is it possible to perform two different operations on the same volume at once? Only, then the above-mentioned scenario should occur, right? Like, what I understood is if one thread is accessing the volume, fetching the volinfo, and performing further tasks, in the meantime some other thread can remove that volinfo from the table that would make the fetched volinfo in the other thread useless. As far as I know, we can perform only one operation at some point in time on a volume, but operations can be done on other volumes. Please pardon me, if I understood something wrong.

[xhernandez]

You are using the volinfo outside the read-lock section. In glusterd_volinfo_find() you search the volume inside the read-lock section, but once you find it, the read-lock section terminates and the volinfo is returned. This means that callers of glusterd_volinfo_find() can be accessing a destroyed element if another thread has removed the volinfo from the list.

Okay, I get your point. But I have one question like is it possible to perform two different operations on the same volume at once? Only, then the above-mentioned scenario should occur, right? Like, what I understood is if one thread is accessing the volume, fetching the volinfo, and performing further tasks, in the meantime some other thread can remove that volinfo from the table that would make the fetched volinfo in the other thread useless. As far as I know, we can perform only one operation at some point in time on a volume, but operations can be done on other volumes. Please pardon me, if I understood something wrong.

I don't know enough how glusterd works internally, but if it's true that it's not possible to remove a volume from the hash while another operation is being processed, then why do we need synchronization ? we can use a simple hash table without RCU nor its requirements.

If you use RCU to protect a structure, it's assumed that deletes and additions can happen concurrently with lookups and, in this case, RCU must be used properly. If this is not really needed, the first thing to do is to avoid using RCU-based structures.

I can't tell if glusterd could really modify and lookup the hash table at the same time, but IMO if we use RCU, we must use it correctly. Otherwise explicitly document why this cannot happen and use a simpler structure.

[nik-redhat]

I don't know enough how glusterd works internally, but if it's true that it's not possible to remove a volume from the hash while another operation is being processed, then why do we need synchronization ? we can use a simple hash table without RCU nor its requirements.

Multiple operations on the same volume can't be performed, but multiple transactions on different volumes can be executed at the same time.

@mohit84 if you could please help out or add your views in this discussion as well, it would be very helpful.

[nik-redhat]

/run regression

[nik-redhat]

/run regression

[nik-redhat]

/run regression

[nik-redhat]

/run regression

[nik-redhat]

/run regression

[nik-redhat]

/run regression

[nik-redhat]

/run regression

[nik-redhat]

/run regression

[nik-redhat]

/run regression

[nik-redhat]

/run regression

[nik-redhat]

/run regression

[nik-redhat]

/run regression

[nik-redhat]

/run regression

[nik-redhat]

/run regression

[nik-redhat]

/run regression

[nik-redhat]

/run regression

[nik-redhat]

/run regression

[nik-redhat]

/run regression

[nik-redhat]

/run regression

[nik-redhat]

/run regression

[nik-redhat]

/recheck smoke

[nik-redhat]

/run regression

[nik-redhat]

/run regression

[stale[bot]]

Thank you for your contributions. Noticed that this issue is not having any activity in last ~6 months! We are marking this issue as stale because it has not had recent activity. It will be closed in 2 weeks if no one responds with a comment here.

[stale[bot]]

Closing this issue as there was no update since my last update on issue. If this is an issue which is still valid, feel free to open it.