r15614: the byte range locking error handling caches the last failed lock

per file handle and not per tree connect

metze
(This used to be commit 5d825261c0b8341f0a7f0f6d96d83807352566f4)

4 files changed, 133 insertions, 85 deletions

diff --git a/source4/ntvfs/common/brlock.c b/source4/ntvfs/common/brlock.c
index 07f1593c2e..b5df434435 100644
--- a/source4/ntvfs/common/brlock.c
+++ b/source4/ntvfs/common/brlock.c
@@ -57,21 +57,27 @@ struct lock_context {
    size of the record */
 struct lock_struct {
 	struct lock_context context;
+	uint16_t fnum;
 	uint64_t start;
 	uint64_t size;
-	uint16_t fnum;
 	enum brl_type lock_type;
 	void *notify_ptr;
 };
 
+/* this struct is attached to on oprn file handle */
+struct brl_handle {
+	DATA_BLOB key;
+	uint16_t fnum;
+	struct lock_struct last_lock;
+};
+
+/* this struct is typicaly attached to tcon */
 struct brl_context {
 	struct tdb_wrap *w;
 	uint32_t server;
 	struct messaging_context *messaging_ctx;
-	struct lock_struct last_lock;
 };
 
-
 /*
   Open up the brlock.tdb database. Close it down using
   talloc_free(). We need the messaging_ctx to allow for
@@ -89,7 +95,7 @@ struct brl_context *brl_init(TALLOC_CTX *mem_ctx, uint32_t server,
 	}
 
 	path = smbd_tmp_path(brl, "brlock.tdb");
-	brl->w = tdb_wrap_open(brl, path, 0,  
+	brl->w = tdb_wrap_open(brl, path, 0,
 			       TDB_DEFAULT, O_RDWR|O_CREAT, 0600);
 	talloc_free(path);
 	if (brl->w == NULL) {
@@ -99,11 +105,25 @@ struct brl_context *brl_init(TALLOC_CTX *mem_ctx, uint32_t server,
 
 	brl->server = server;
 	brl->messaging_ctx = messaging_ctx;
-	ZERO_STRUCT(brl->last_lock);
 
 	return brl;
 }
 
+struct brl_handle *brl_create_handle(TALLOC_CTX *mem_ctx, DATA_BLOB *file_key, uint16_t fnum)
+{
+	struct brl_handle *brlh;
+
+	brlh = talloc(mem_ctx, struct brl_handle);
+	if (brlh == NULL) {
+		return NULL;
+	}
+
+	brlh->key = *file_key;
+	brlh->fnum = fnum;
+	ZERO_STRUCT(brlh->last_lock);
+
+	return brlh;
+}
 
 /*
   see if two locking contexts are equal
@@ -196,24 +216,47 @@ static BOOL brl_conflict_other(struct lock_struct *lck1, struct lock_struct *lck
   is the same as this one and changes its error code. I wonder if any
   app depends on this?
 */
-static NTSTATUS brl_lock_failed(struct brl_context *brl, struct lock_struct *lock)
+static NTSTATUS brl_lock_failed(struct brl_handle *brlh, struct lock_struct *lock)
 {
-	if (lock->context.server == brl->last_lock.context.server &&
-	    lock->context.ctx == brl->last_lock.context.ctx &&
-	    lock->fnum == brl->last_lock.fnum &&
-	    lock->start == brl->last_lock.start &&
-	    lock->size == brl->last_lock.size) {
+	/*
+	 * this function is only called for non pending lock!
+	 */
+
+	/* 
+	 * if the notify_ptr is non NULL,
+	 * it means that we're at the end of a pending lock
+	 * and the real lock is requested after the timout went by
+	 * In this case we need to remember the last_lock and always
+	 * give FILE_LOCK_CONFLICT
+	 */
+	if (lock->notify_ptr) {
+		brlh->last_lock = *lock;
 		return NT_STATUS_FILE_LOCK_CONFLICT;
 	}
-	brl->last_lock = *lock;
-	if (lock->start >= 0xEF000000 && 
-	    (lock->start >> 63) == 0) {
-		/* amazing the little things you learn with a test
-		   suite. Locks beyond this offset (as a 64 bit
-		   number!) always generate the conflict error code,
-		   unless the top bit is set */
+
+	/* 
+	 * amazing the little things you learn with a test
+	 * suite. Locks beyond this offset (as a 64 bit
+	 * number!) always generate the conflict error code,
+	 * unless the top bit is set
+	 */
+	if (lock->start >= 0xEF000000 && (lock->start >> 63) == 0) {
+		brlh->last_lock = *lock;
 		return NT_STATUS_FILE_LOCK_CONFLICT;
 	}
+
+	/*
+	 * if the current lock matches the last failed lock on the file handle
+	 * and starts at the same offset, then FILE_LOCK_CONFLICT should be returned
+	 */
+	if (lock->context.server == brlh->last_lock.context.server &&
+	    lock->context.ctx == brlh->last_lock.context.ctx &&
+	    lock->fnum == brlh->last_lock.fnum &&
+	    lock->start == brlh->last_lock.start) {
+		return NT_STATUS_FILE_LOCK_CONFLICT;
+	}
+
+	brlh->last_lock = *lock;
 	return NT_STATUS_LOCK_NOT_GRANTED;
 }
 
@@ -225,9 +268,8 @@ static NTSTATUS brl_lock_failed(struct brl_context *brl, struct lock_struct *loc
   notification is sent, identified by the notify_ptr
 */
 NTSTATUS brl_lock(struct brl_context *brl,
-		  DATA_BLOB *file_key, 
+		  struct brl_handle *brlh,
 		  uint16_t smbpid,
-		  uint16_t fnum, 
 		  uint64_t start, uint64_t size, 
 		  enum brl_type lock_type,
 		  void *notify_ptr)
@@ -237,8 +279,8 @@ NTSTATUS brl_lock(struct brl_context *brl,
 	struct lock_struct lock, *locks=NULL;
 	NTSTATUS status;
 
-	kbuf.dptr = file_key->data;
-	kbuf.dsize = file_key->length;
+	kbuf.dptr = brlh->key.data;
+	kbuf.dsize = brlh->key.length;
 
 	if (tdb_chainlock(brl->w->tdb, kbuf) != 0) {
 		return NT_STATUS_INTERNAL_DB_CORRUPTION;
@@ -251,7 +293,12 @@ NTSTATUS brl_lock(struct brl_context *brl,
 	   preventing the real lock gets removed */
 	if (lock_type >= PENDING_READ_LOCK) {
 		enum brl_type rw = (lock_type==PENDING_READ_LOCK? READ_LOCK : WRITE_LOCK);
-		status = brl_lock(brl, file_key, smbpid, fnum, start, size, rw, NULL);
+
+		/* here we need to force that the last_lock isn't overwritten */
+		lock = brlh->last_lock;
+		status = brl_lock(brl, brlh, smbpid, start, size, rw, NULL);
+		brlh->last_lock = lock;
+
 		if (NT_STATUS_IS_OK(status)) {
 			tdb_chainunlock(brl->w->tdb, kbuf);
 			return NT_STATUS_OK;
@@ -263,9 +310,10 @@ NTSTATUS brl_lock(struct brl_context *brl,
 	lock.context.smbpid = smbpid;
 	lock.context.server = brl->server;
 	lock.context.ctx = brl;
+	lock.fnum = brlh->fnum;
+	lock.context.ctx = brl;
 	lock.start = start;
 	lock.size = size;
-	lock.fnum = fnum;
 	lock.lock_type = lock_type;
 	lock.notify_ptr = notify_ptr;
 
@@ -275,7 +323,7 @@ NTSTATUS brl_lock(struct brl_context *brl,
 		count = dbuf.dsize / sizeof(*locks);
 		for (i=0; i<count; i++) {
 			if (brl_conflict(&locks[i], &lock)) {
-				status = brl_lock_failed(brl, &lock);
+				status = brl_lock_failed(brlh, &lock);
 				goto fail;
 			}
 		}
@@ -304,7 +352,7 @@ NTSTATUS brl_lock(struct brl_context *brl,
 	   we have reached here then it must be a pending lock that
 	   was granted, so tell them the lock failed */
 	if (lock_type >= PENDING_READ_LOCK) {
-		return brl_lock_failed(brl, &lock);
+		return NT_STATUS_LOCK_NOT_GRANTED;
 	}
 
 	return NT_STATUS_OK;
@@ -371,9 +419,8 @@ static void brl_notify_all(struct brl_context *brl,
  Unlock a range of bytes.
 */
 NTSTATUS brl_unlock(struct brl_context *brl,
-		    DATA_BLOB *file_key, 
+		    struct brl_handle *brlh, 
 		    uint16_t smbpid,
-		    uint16_t fnum,
 		    uint64_t start, uint64_t size)
 {
 	TDB_DATA kbuf, dbuf;
@@ -382,8 +429,8 @@ NTSTATUS brl_unlock(struct brl_context *brl,
 	struct lock_context context;
 	NTSTATUS status;
 
-	kbuf.dptr = file_key->data;
-	kbuf.dsize = file_key->length;
+	kbuf.dptr = brlh->key.data;
+	kbuf.dsize = brlh->key.length;
 
 	if (tdb_chainlock(brl->w->tdb, kbuf) != 0) {
 		return NT_STATUS_INTERNAL_DB_CORRUPTION;
@@ -407,7 +454,7 @@ NTSTATUS brl_unlock(struct brl_context *brl,
 		struct lock_struct *lock = &locks[i];
 		
 		if (brl_same_context(&lock->context, &context) &&
-		    lock->fnum == fnum &&
+		    lock->fnum == brlh->fnum &&
 		    lock->start == start &&
 		    lock->size == size &&
 		    lock->lock_type < PENDING_READ_LOCK) {
@@ -458,7 +505,7 @@ NTSTATUS brl_unlock(struct brl_context *brl,
   getting it. In either case they no longer need to be notified.
 */
 NTSTATUS brl_remove_pending(struct brl_context *brl,
-			    DATA_BLOB *file_key, 
+			    struct brl_handle *brlh, 
 			    void *notify_ptr)
 {
 	TDB_DATA kbuf, dbuf;
@@ -466,8 +513,8 @@ NTSTATUS brl_remove_pending(struct brl_context *brl,
 	struct lock_struct *locks;
 	NTSTATUS status;
 
-	kbuf.dptr = file_key->data;
-	kbuf.dsize = file_key->length;
+	kbuf.dptr = brlh->key.data;
+	kbuf.dsize = brlh->key.length;
 
 	if (tdb_chainlock(brl->w->tdb, kbuf) != 0) {
 		return NT_STATUS_INTERNAL_DB_CORRUPTION;
@@ -528,8 +575,7 @@ NTSTATUS brl_remove_pending(struct brl_context *brl,
   Test if we are allowed to perform IO on a region of an open file
 */
 NTSTATUS brl_locktest(struct brl_context *brl,
-		      DATA_BLOB *file_key, 
-		      uint16_t fnum,
+		      struct brl_handle *brlh,
 		      uint16_t smbpid, 
 		      uint64_t start, uint64_t size, 
 		      enum brl_type lock_type)
@@ -538,8 +584,8 @@ NTSTATUS brl_locktest(struct brl_context *brl,
 	int count, i;
 	struct lock_struct lock, *locks;
 
-	kbuf.dptr = file_key->data;
-	kbuf.dsize = file_key->length;
+	kbuf.dptr = brlh->key.data;
+	kbuf.dsize = brlh->key.length;
 
 	dbuf = tdb_fetch(brl->w->tdb, kbuf);
 	if (dbuf.dptr == NULL) {
@@ -549,9 +595,9 @@ NTSTATUS brl_locktest(struct brl_context *brl,
 	lock.context.smbpid = smbpid;
 	lock.context.server = brl->server;
 	lock.context.ctx = brl;
+	lock.fnum = brlh->fnum;
 	lock.start = start;
 	lock.size = size;
-	lock.fnum = fnum;
 	lock.lock_type = lock_type;
 
 	/* there are existing locks - make sure they don't conflict */
@@ -574,15 +620,15 @@ NTSTATUS brl_locktest(struct brl_context *brl,
  Remove any locks associated with a open file.
 */
 NTSTATUS brl_close(struct brl_context *brl,
-		   DATA_BLOB *file_key, int fnum)
+		   struct brl_handle *brlh)
 {
 	TDB_DATA kbuf, dbuf;
 	int count, i, dcount=0;
 	struct lock_struct *locks;
 	NTSTATUS status;
 
-	kbuf.dptr = file_key->data;
-	kbuf.dsize = file_key->length;
+	kbuf.dptr = brlh->key.data;
+	kbuf.dsize = brlh->key.length;
 
 	if (tdb_chainlock(brl->w->tdb, kbuf) != 0) {
 		return NT_STATUS_INTERNAL_DB_CORRUPTION;
@@ -603,7 +649,7 @@ NTSTATUS brl_close(struct brl_context *brl,
 
 		if (lock->context.ctx == brl &&
 		    lock->context.server == brl->server &&
-		    lock->fnum == fnum) {
+		    lock->fnum == brlh->fnum) {
 			/* found it - delete it */
 			if (count > 1 && i < count-1) {
 				memmove(&locks[i], &locks[i+1], 
diff --git a/source4/ntvfs/posix/pvfs_lock.c b/source4/ntvfs/posix/pvfs_lock.c
index 0558fd52ea..99b694665d 100644
--- a/source4/ntvfs/posix/pvfs_lock.c
+++ b/source4/ntvfs/posix/pvfs_lock.c
@@ -41,8 +41,7 @@ NTSTATUS pvfs_check_lock(struct pvfs_state *pvfs,
 	}
 
 	return brl_locktest(pvfs->brl_context,
-			    &f->handle->brl_locking_key,
-			    f->fnum,
+			    f->brl_handle,
 			    smbpid,
 			    offset, count, rw);
 }
@@ -73,9 +72,8 @@ static void pvfs_lock_async_failed(struct pvfs_state *pvfs,
 	/* undo the locks we just did */
 	for (i=i-1;i>=0;i--) {
 		brl_unlock(pvfs->brl_context,
-			   &f->handle->brl_locking_key,
+			   f->brl_handle,
 			   locks[i].pid,
-			   f->fnum,
 			   locks[i].offset,
 			   locks[i].count);
 		f->lock_count--;
@@ -120,13 +118,17 @@ static void pvfs_pending_lock_continue(void *private, enum pvfs_wait_notice reas
 	if (reason == PVFS_WAIT_CANCEL) {
 		status = NT_STATUS_FILE_LOCK_CONFLICT;
 	} else {
+		/* 
+		 * here it's important to pass the pending pointer
+		 * because with this we'll get the correct error code
+		 * FILE_LOCK_CONFLICT in the error case
+		 */
 		status = brl_lock(pvfs->brl_context,
-				  &f->handle->brl_locking_key,
+				  f->brl_handle,
 				  req->smbpid,
-				  f->fnum,
 				  locks[pending->pending_lock].offset,
 				  locks[pending->pending_lock].count,
-				  rw, NULL);
+				  rw, pending);
 	}
 	if (NT_STATUS_IS_OK(status)) {
 		f->lock_count++;
@@ -138,7 +140,7 @@ static void pvfs_pending_lock_continue(void *private, enum pvfs_wait_notice reas
 	if (NT_STATUS_IS_OK(status) || timed_out) {
 		NTSTATUS status2;
 		status2 = brl_remove_pending(pvfs->brl_context, 
-					     &f->handle->brl_locking_key, pending);
+					     f->brl_handle, pending);
 		if (!NT_STATUS_IS_OK(status2)) {
 			DEBUG(0,("pvfs_lock: failed to remove pending lock - %s\n", nt_errstr(status2)));
 		}
@@ -171,9 +173,8 @@ static void pvfs_pending_lock_continue(void *private, enum pvfs_wait_notice reas
 		}
 
 		status = brl_lock(pvfs->brl_context,
-				  &f->handle->brl_locking_key,
+				  f->brl_handle,
 				  req->smbpid,
-				  f->fnum,
 				  locks[i].offset,
 				  locks[i].count,
 				  rw, pending);
@@ -216,7 +217,7 @@ void pvfs_lock_close(struct pvfs_state *pvfs, struct pvfs_file *f)
 	if (f->lock_count || f->pending_list) {
 		DEBUG(5,("pvfs_lock: removing %.0f locks on close\n", 
 			 (double)f->lock_count));
-		brl_close(f->pvfs->brl_context, &f->handle->brl_locking_key, f->fnum);
+		brl_close(f->pvfs->brl_context, f->brl_handle);
 		f->lock_count = 0;
 	}
 
@@ -338,9 +339,8 @@ NTSTATUS pvfs_lock(struct ntvfs_module_context *ntvfs,
 
 	for (i=0;i<lck->lockx.in.ulock_cnt;i++) {
 		status = brl_unlock(pvfs->brl_context,
-				    &f->handle->brl_locking_key,
+				    f->brl_handle,
 				    locks[i].pid,
-				    f->fnum,
 				    locks[i].offset,
 				    locks[i].count);
 		if (!NT_STATUS_IS_OK(status)) {
@@ -357,9 +357,8 @@ NTSTATUS pvfs_lock(struct ntvfs_module_context *ntvfs,
 		}
 
 		status = brl_lock(pvfs->brl_context,
-				  &f->handle->brl_locking_key,
+				  f->brl_handle,
 				  locks[i].pid,
-				  f->fnum,
 				  locks[i].offset,
 				  locks[i].count,
 				  rw, pending);
@@ -381,9 +380,8 @@ NTSTATUS pvfs_lock(struct ntvfs_module_context *ntvfs,
 			/* undo the locks we just did */
 			for (i=i-1;i>=0;i--) {
 				brl_unlock(pvfs->brl_context,
-					   &f->handle->brl_locking_key,
+					   f->brl_handle,
 					   locks[i].pid,
-					   f->fnum,
 					   locks[i].offset,
 					   locks[i].count);
 				f->lock_count--;
@@ -395,4 +393,3 @@ NTSTATUS pvfs_lock(struct ntvfs_module_context *ntvfs,
 
 	return NT_STATUS_OK;
 }
-
diff --git a/source4/ntvfs/posix/pvfs_open.c b/source4/ntvfs/posix/pvfs_open.c
index 9570fa08d9..3bbf840154 100644
--- a/source4/ntvfs/posix/pvfs_open.c
+++ b/source4/ntvfs/posix/pvfs_open.c
@@ -277,13 +277,13 @@ static NTSTATUS pvfs_open_directory(struct pvfs_state *pvfs,
 	f->share_access  = io->generic.in.share_access;
 	f->impersonation = io->generic.in.impersonation;
 	f->access_mask   = access_mask;
+	f->brl_handle	 = NULL;
 	f->notify_buffer = NULL;
 
 	f->handle->pvfs              = pvfs;
 	f->handle->name              = talloc_steal(f->handle, name);
 	f->handle->fd                = -1;
 	f->handle->odb_locking_key   = data_blob(NULL, 0);
-	f->handle->brl_locking_key   = data_blob(NULL, 0);
 	f->handle->create_options    = io->generic.in.create_options;
 	f->handle->seek_offset       = 0;
 	f->handle->position          = 0;
@@ -526,32 +526,37 @@ static int pvfs_fnum_destructor(void *p)
   account of file streams (each stream is a separate byte range
   locking space)
 */
-static NTSTATUS pvfs_brl_locking_key(struct pvfs_filename *name, 
-				     TALLOC_CTX *mem_ctx, DATA_BLOB *key)
+static NTSTATUS pvfs_brl_locking_handle(TALLOC_CTX *mem_ctx,
+					struct pvfs_filename *name, 
+					uint16_t fnum,
+					struct brl_handle **_h)
 {
-	DATA_BLOB odb_key;
+	DATA_BLOB odb_key, key;
 	NTSTATUS status;
+	struct brl_handle *h;
+
 	status = pvfs_locking_key(name, mem_ctx, &odb_key);
-	if (!NT_STATUS_IS_OK(status)) {
-		return status;
-	}
+	NT_STATUS_NOT_OK_RETURN(status);
+
 	if (name->stream_name == NULL) {
-		*key = odb_key;
-		return NT_STATUS_OK;
-	}
-	*key = data_blob_talloc(mem_ctx, NULL, 
-				odb_key.length + strlen(name->stream_name) + 1);
-	if (key->data == NULL) {
-		return NT_STATUS_NO_MEMORY;
+		key = odb_key;
+	} else {
+		key = data_blob_talloc(mem_ctx, NULL, 
+				       odb_key.length + strlen(name->stream_name) + 1);
+		NT_STATUS_HAVE_NO_MEMORY(key.data);
+		memcpy(key.data, odb_key.data, odb_key.length);
+		memcpy(key.data + odb_key.length, 
+		       name->stream_name, strlen(name->stream_name) + 1);
+		data_blob_free(&odb_key);
 	}
-	memcpy(key->data, odb_key.data, odb_key.length);
-	memcpy(key->data + odb_key.length, 
-	       name->stream_name, strlen(name->stream_name)+1);
-	data_blob_free(&odb_key);
+
+	h = brl_create_handle(mem_ctx, &key, fnum);
+	NT_STATUS_HAVE_NO_MEMORY(h);
+
+	*_h = h;
 	return NT_STATUS_OK;
 }
 
-
 /*
   create a new file
 */
@@ -665,7 +670,7 @@ static NTSTATUS pvfs_create_file(struct pvfs_state *pvfs,
 		goto cleanup_delete;
 	}
 
-	status = pvfs_brl_locking_key(name, f->handle, &f->handle->brl_locking_key);
+	status = pvfs_brl_locking_handle(f, name, fnum, &f->brl_handle);
 	if (!NT_STATUS_IS_OK(status)) {
 		goto cleanup_delete;
 	}
@@ -1168,7 +1173,7 @@ NTSTATUS pvfs_open(struct ntvfs_module_context *ntvfs,
 		return status;
 	}
 
-	status = pvfs_brl_locking_key(name, f->handle, &f->handle->brl_locking_key);
+	status = pvfs_brl_locking_handle(f, name, f->fnum, &f->brl_handle);
 	if (!NT_STATUS_IS_OK(status)) {
 		idr_remove(pvfs->files.idtree, f->fnum);
 		return status;
diff --git a/source4/ntvfs/posix/vfs_posix.h b/source4/ntvfs/posix/vfs_posix.h
index 335cfdf4e0..39481c03b1 100644
--- a/source4/ntvfs/posix/vfs_posix.h
+++ b/source4/ntvfs/posix/vfs_posix.h
@@ -134,9 +134,6 @@ struct pvfs_file_handle {
 	/* a unique file key to be used for open file locking */
 	DATA_BLOB odb_locking_key;
 
-	/* a unique file key to be used for byte range locking */
-	DATA_BLOB brl_locking_key;
-
 	uint32_t create_options;
 
 	/* this is set by the mode_information level. What does it do? */
@@ -178,6 +175,9 @@ struct pvfs_file {
 	/* a list of pending locks - used for locking cancel operations */
 	struct pvfs_pending_lock *pending_list;
 
+	/* a file handle to be used for byte range locking */
+	struct brl_handle *brl_handle;
+
 	/* a count of active locks - used to avoid calling brl_close on
 	   file close */
 	uint64_t lock_count;