<div dir="ltr"><div dir="ltr"><br></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Sun, Mar 31, 2019 at 11:29 PM Soumya Koduri &lt;<a href="mailto:skoduri@redhat.com">skoduri@redhat.com</a>&gt; wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><br>
<br>
On 3/29/19 11:55 PM, Xavi Hernandez wrote:<br>
&gt; Hi all,<br>
&gt; <br>
&gt; there is one potential problem with posix locks when used in a <br>
&gt; replicated or dispersed volume.<br>
&gt; <br>
&gt; Some background:<br>
&gt; <br>
&gt; Posix locks allow any process to lock a region of a file multiple times, <br>
&gt; but a single unlock on a given region will release all previous locks. <br>
&gt; Locked regions can be different for each lock request and they can <br>
&gt; overlap. The resulting lock will cover the union of all locked regions. <br>
&gt; A single unlock (the region doesn&#39;t necessarily need to match any of the <br>
&gt; ranges used for locking) will create a &quot;hole&quot; in the currently locked <br>
&gt; region, independently of how many times a lock request covered that region.<br>
&gt; <br>
&gt; For this reason, the locks xlator simply combines the locked regions <br>
&gt; that are requested, but it doesn&#39;t track each individual lock range.<br>
&gt; <br>
&gt; Under normal circumstances this works fine. But there are some cases <br>
&gt; where this behavior is not sufficient. For example, suppose we have a <br>
&gt; replica 3 volume with quorum = 2. Given the special nature of posix <br>
&gt; locks, AFR sends the lock request sequentially to each one of the <br>
&gt; bricks, to avoid that conflicting lock requests from other clients could <br>
&gt; require to unlock an already locked region on the client that has not <br>
&gt; got enough successful locks (i.e. quorum). An unlock here not only would <br>
&gt; cancel the current lock request. It would also cancel any previously <br>
&gt; acquired lock.<br>
&gt; <br>
<br>
I may not have fully understood, please correct me. AFAIU, lk xlator <br>
merges locks only if both the lk-owner and the client opaque matches.<br>
<br>
In the case which you have mentioned above, considering clientA acquired <br>
locks on majority of quorum (say nodeA and nodeB) and clientB on nodeC <br>
alone- clientB now has to unlock/cancel the lock it acquired on nodeC.<br>
<br>
You are saying the it could pose a problem if there were already <br>
successful locks taken by clientB for the same region which would get <br>
unlocked by this particular unlock request..right?<br>
<br>
Assuming the previous locks acquired by clientB are shared (otherwise <br>
clientA wouldn&#39;t have got granted lock for the same region on nodeA &amp; <br>
nodeB), they would still hold true on nodeA &amp; nodeBĀ  as the unlock <br>
request was sent to only nodeC. Since the majority of quorum nodes still <br>
hold the locks by clientB, this isn&#39;t serious issue IMO.<br>
<br>
I haven&#39;t looked into heal part but would like to understand if this is <br>
really an issue in normal scenarios as well.<br></blockquote><div><br></div><div>This is how I understood the code. Consider the following case:</div><div>Nodes A, B, C have locks with start and end offsets: 5-15 from mount-1 and lock-range 2-3 from mount-2.</div><div>If mount-1 requests nonblocking lock with lock-range 1-7 and in parallel lets say mount-2 issued unlock of 2-3 as well.<br></div><div><br></div><div>nodeA got unlock from mount-2 with range 2-3 then lock from mount-1 with range 1-7, so the lock is granted and merged to give 1-15</div><div>nodeB got lock from mount-1 with range 1-7 before unlock of 2-3 which leads to EAGAIN which will trigger unlocks on granted lock in mount-1 which will end up doing unlock of 1-7 on nodeA leading to lock-range 8-15 instead of the original 5-15 on nodeA. Whereas nodeB and nodeC will have range 5-15.</div><div><br></div><div>Let me know if my understanding is wrong.</div><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
Thanks,<br>
Soumya<br>
<br>
&gt; However, when something goes wrong (a brick dies during a lock request, <br>
&gt; or there&#39;s a network partition or some other weird situation), it could <br>
&gt; happen that even using sequential locking, only one brick succeeds the <br>
&gt; lock request. In this case, AFR should cancel the previous lock (and it <br>
&gt; does), but this also cancels any previously acquired lock on that <br>
&gt; region, which is not good.<br>
&gt; <br>
&gt; A similar thing can happen if we try to recover (heal) posix locks that <br>
&gt; were active after a brick has been disconnected (for any reason) and <br>
&gt; then reconnected.<br>
&gt; <br>
&gt; To fix all these situations we need to change the way posix locks are <br>
&gt; managed by locks xlator. One possibility would be to embed the lock <br>
&gt; request inside an inode transaction using inodelk. Since inodelks do not <br>
&gt; suffer this problem, the follwing posix lock could be sent safely. <br>
&gt; However this implies an additional network request, which could cause <br>
&gt; some performance impact. Eager-locking could minimize the impact in some <br>
&gt; cases. However this approach won&#39;t work for lock recovery after a <br>
&gt; disconnect.<br>
&gt; <br>
&gt; Another possibility is to send a special partial posix lock request <br>
&gt; which won&#39;t be immediately merged with already existing locks once <br>
&gt; granted. An additional confirmation request of the partial posix lock <br>
&gt; will be required to fully grant the current lock and merge it with the <br>
&gt; existing ones. This requires a new network request, which will add <br>
&gt; latency, and makes everything more complex since there would be more <br>
&gt; combinations of states in which something could fail.<br>
&gt; <br>
&gt; So I think one possible solution would be the following:<br>
&gt; <br>
&gt; 1. Keep each posix lock as an independent object in locks xlator. This <br>
&gt; will make it possible to &quot;invalidate&quot; any already granted lock without <br>
&gt; affecting already established locks.<br>
&gt; <br>
&gt; 2. Additionally, we&#39;ll keep a sorted list of non-overlapping segments of <br>
&gt; locked regions. And we&#39;ll count, for each region, how many locks are <br>
&gt; referencing it. One lock can reference multiple segments, and each <br>
&gt; segment can be referenced by multiple locks.<br>
&gt; <br>
&gt; 3. An additional lock request that overlaps with an existing segment, <br>
&gt; can cause this segment to be split to satisfy the non-overlapping property.<br>
&gt; <br>
&gt; 4. When an unlock request is received, all segments intersecting with <br>
&gt; the region are eliminated (it may require some segment splits on the <br>
&gt; edges), and the unlocked region is subtracted from each lock associated <br>
&gt; to the segment. If a lock gets an empty region, it&#39;s removed.<br>
&gt; <br>
&gt; 5. We&#39;ll create a special &quot;remove lock&quot; request that doesn&#39;t unlock a <br>
&gt; region but removes an already granted lock. This will decrease the <br>
&gt; number of references to each of the segments this lock was covering. If <br>
&gt; some segment reaches 0, it&#39;s removed. Otherwise it remains there. This <br>
&gt; special request will only be used internally to cancel already acquired <br>
&gt; locks that cannot be fully granted due to quorum issues or any other <br>
&gt; problem.<br>
&gt; <br>
&gt; In some weird cases, the list of segments can be huge (many locks <br>
&gt; overlapping only on a single byte, so each segment represents only one <br>
&gt; byte). We can try to find some smarter structure that minimizes this <br>
&gt; problem or limit the number of segments (for example returning ENOLCK <br>
&gt; when there are too many).<br>
&gt; <br>
&gt; What do you think ?<br>
&gt; <br>
&gt; Xavi<br>
&gt; <br>
&gt; _______________________________________________<br>
&gt; Gluster-devel mailing list<br>
&gt; <a href="mailto:Gluster-devel@gluster.org" target="_blank">Gluster-devel@gluster.org</a><br>
&gt; <a href="https://lists.gluster.org/mailman/listinfo/gluster-devel" rel="noreferrer" target="_blank">https://lists.gluster.org/mailman/listinfo/gluster-devel</a><br>
&gt; <br>
</blockquote></div><br clear="all"><br>-- <br><div dir="ltr" class="gmail_signature"><div dir="ltr">Pranith<br></div></div></div>