On 05/04/2011 12:50 PM, Edward Shishkin wrote:
Hello everyone.
I find the subject suspicious.
See comments in the function below.
Where am I wrong?
Any ideas?
Hi, Edward. Nice to hear from you at last. How are things going?
Yes, there is a race here. If "thread B" does manage to do both a lock
and a read between the time that inode->gen gets incremented and when
the data actually get modified, then it would not recognize that a
conflict had occurred, and would overwrite A's data. Note that B is not
just a local thread but a remote client which would have to incur two
network round trips (the lock and the read) within that window.
Nonetheless, a race is a race . . . but that's not the only problem with
this code. Another problem is false conflicts and spurious retries
(with no guarantee of forward progress) due to the file-level
synchronization scope. This includes the case where the false conflict
is a self-conflict, due e.g. to the way that the "writeback" translator
issues parallel writes. This is why I have suggested before that we
should probably move away from a client-side optimistic-locking/retry
model to a server-side queuing mode, so that two conflicting
read-modify-write sequences *can't* collide at the write phase. We
should probably also look at combining the mainline and "aes" code
(that's wbe issued hy it's a branch) and consider having the
read-modify-writes apply to IVs rather than actual data. I'd certainly
appreciate your thoughts on the best high-level design, if you can find
the time.
Ultima
Thanks,
Edward.
int32_t
oplock_writev (call_frame_t *frame, xlator_t *this, fd_t *fd, struct
iovec *vector,
int32_t count, off_t offset, struct iobref *iobref)
{
server_state_t *state = CALL_STATE(frame);
inode_t *inode = fd->inode;
struct oplock_entry *entry = NULL;
oplock_private_t *priv = this->private;
int op_errno = 0;
entry = fetch_op_lock(&priv->locks,inode,state->conn);
if (entry) {
LIST_REMOVE(entry,links);
if (entry->value != inode->gen) {
gf_log(this->name,GF_LOG_DEBUG,
"would reject write for %d from %p",
inode->ino, state->conn);
op_errno = EBUSY;
goto err;
}
FREE(entry);
}
++(inode->gen);
/* Suppose thread A overwrites a byte at offset 0,
* and thread B overwrites a byte in the same file
* at offset 15 (the same block).
*
* Suppose A sets the counter above (inode->gen) to N.
* At this point global file's data is not yet modified.
* Thread B issues a lock with assigned value N, then
* successfully reads old data from disk.
*/
STACK_WIND (frame, oplock_writev_cbk,
FIRST_CHILD(this), FIRST_CHILD(this)->fops->writev,
fd, vector, count, offset, iobref);
/* At this point global file's data has been modified by A.
* Next, B increases the counter to N+1, and also modifies
* global data. Changes performed by A are lost.
*/
return 0;
err:
STACK_UNWIND_STRICT(writev,frame,-1,op_errno,NULL,NULL);
return 0;
}
_______________________________________________
cloudfs-devel mailing list
cloudfs-devel(a)lists.fedorahosted.org
https://fedorahosted.org/mailman/listinfo/cloudfs-devel