ceph_lock_op2 is set lock implement have a bug?

[JYang1986]

node1： when I in client1 set file range lock of 1.txt in ceph cluster by nfs-gansha1 server，
node2： then in node2 client2 set the same range of 1.txt by nfs-gansha2 server success.
then I see the implement of ceph_lock_op2, is some different from glusterfs_lock_op2

ceph_lock_op2

fsal_status_t ceph_lock_op2(struct fsal_obj_handle *obj_hdl,
			    struct state_t *state,
			    void *owner,
			    fsal_lock_op_t lock_op,
			    fsal_lock_param_t *request_lock,
			    fsal_lock_param_t *conflicting_lock)
{
	struct handle *myself = container_of(obj_hdl, struct handle, handle);
	struct flock lock_args;
...
	if (lock_op == FSAL_OP_LOCKT) {
		retval = ceph_ll_getlk(export->cmount, my_fd, &lock_args,
				       (uint64_t) owner);  
	} else {
		retval = ceph_ll_setlk(export->cmount, my_fd, &lock_args,
				       (uint64_t) owner, false); this retval = -11;
	}

	if (retval < 0) {
		LogDebug(COMPONENT_FSAL,
			 "%s returned %d %s",
			 lock_op == FSAL_OP_LOCKT
				? "ceph_ll_getlk" : "ceph_ll_setlk",
			 -retval, strerror(-retval));

		if (conflicting_lock != NULL) {
			/* Get the conflicting lock */
			retval = ceph_ll_getlk(export->cmount, my_fd,
					       &lock_args, (uint64_t) owner);   this get conflicting_lock success,then retval set by 0_

			if (retval < 0) {
				LogCrit(COMPONENT_FSAL,
					"After failing a lock request, I couldn't even get the details of who owns the lock, error %d %s",
					-retval, strerror(-retval));
				goto err;  set lock failed, return 0  is this ok?
			}

			if (conflicting_lock != NULL) {
				conflicting_lock->lock_length = lock_args.l_len;
				conflicting_lock->lock_start = lock_args.l_start;
				conflicting_lock->lock_type = lock_args.l_type;
			}
		}

		goto err;
	}
....
 err:

	if (closefd)
		(void) ceph_ll_close(myself->export->cmount, my_fd);

	if (has_lock)
		PTHREAD_RWLOCK_unlock(&obj_hdl->obj_lock);

	return ceph2fsal_error(retval);  
}

glusterfs_lock_op2

static fsal_status_t glusterfs_lock_op2(struct fsal_obj_handle *obj_hdl,
					struct state_t *state,
					void *p_owner,
					fsal_lock_op_t lock_op,
					fsal_lock_param_t *request_lock,
					fsal_lock_param_t *conflicting_lock)
{
	struct flock lock_args;
...

	retval = glfs_posix_lock(my_fd.glfd, fcntl_comm, &lock_args);

	if (retval /* && lock_op == FSAL_OP_LOCK */) {
		retval = errno;
		int rc = 0;  gluster use temporary rc to save return value

		LogDebug(COMPONENT_FSAL,
			 "fcntl returned %d %s",
			 retval, strerror(retval));

		if (conflicting_lock != NULL) {
			/* Get the conflicting lock */
			rc = glfs_posix_lock(my_fd.glfd, F_GETLK,
						 &lock_args);

			if (rc) {  if error， then reset the retval by errno.
				retval = errno; /* we lose the initial error */
				LogCrit(COMPONENT_FSAL,
					"After failing a lock request, I couldn't even get the details of who owns the lock.");
				goto err;
			}

			conflicting_lock->lock_length = lock_args.l_len;
			conflicting_lock->lock_start = lock_args.l_start;
			conflicting_lock->lock_type = lock_args.l_type;
		}

		goto err;
	}
...

 err:
	SET_GLUSTER_CREDS(glfs_export, NULL, NULL, 0, NULL);

	if (closefd)
		glusterfs_close_my_fd(&my_fd);

	if (has_lock)
		PTHREAD_RWLOCK_unlock(&obj_hdl->obj_lock);

	return fsalstat(posix2fsal_error(retval), retval);
}

[ffilz]

Hmm, I don't see a substantial difference. There is one simplification that could be made to ceph_lock_op2 for more readability (redundant testing conflicting_lock != NULL inside an if conflicting_lock !=NULL, but of course the compiler will have optimized that out...).

There could be a bug in ceph_ll_setlk...

[JYang1986]

@ffilz
I mean ceph_ll_setlk have a bug。I have write wrong site 。。。。

[jtlayton]

@Saber-Yang can you open a bug at https://tracker.ceph.com ? I'll plan to look at this soon if so.

[JYang1986]

@jtlayton
hello jtlayton，I have another question。why first ceph_ll_getlk failed ， and if conflicting_lock ！= NULL， ceph_ll_getlk again，this is must？

[JYang1986]

@jtlayton
Is this ceph's bug or nfs-ganesha bug? I have open a bug
http://tracker.ceph.com/issues/21413

[jtlayton]

On Fri, 2017-09-15 at 18:49 -0700, Saber-Yang wrote: @jtlayton Is this ceph's bug or nfs-ganesha bug? I have open a bug http://tracker.ceph.com/issues/21413

Thanks for opening the bug. I'm not sure yet where the bug is -- we'll need to do some tests with ceph alone to see if the locking there actually works. I'll grab the bug, but no idea when I'll have time to work on it. Thanks, -- Jeff Layton <jlayton@redhat.com>

[ffilz]

@jlayton - note that nfs-ganesha src/tools/multilock has ml_cephfs_client.c which was directly drive libcephfs for locking.

[jtlayton]

@Saber-Yang was correct initially -- this is a bug in FSAL_CEPH code. The getlk call ends up clobbering the -EAGAIN from the earlier setlk failure. Gerritt Review request here:

https://review.gerrithub.io/#/c/381714/

[JYang1986]

https://review.gerrithub.io/#/c/381714/ is OK. I will close this issue.