From: Volker Lendecke Date: Wed, 11 Sep 2013 12:48:14 +0000 (+0000) Subject: smbd: Put "have_read_oplocks" into brlock.tdb X-Git-Url: http://git.samba.org/?p=mat%2Fsamba.git;a=commitdiff_plain;h=eb50c18c4a2d0caa3b8d21b2e1b536adc8dc0276 smbd: Put "have_read_oplocks" into brlock.tdb This implements an idea by metze: Right now Samba does not grant level2 oplocks where it should: After an initial no-oplock open that has been written to, we don't have the FAKE_LEVEL2_OPLOCK entry in locking.tdb around anymore, this downgraded to NO_OPLOCK. Windows in this case will grant level2 if being asked, we don't. Part of the reason for this is that we don't have a proper mechanism to communicate the fact that level2 needs to be broken to other smbds. Metze's insight was that we have to look into brlock.tdb for every write anyway, so this might be the right place to store this information. My first reaction was that this is really hackish, but on further thought this is not. oplocks depend on brlocks anyway, and we have the proper mechanisms in place for brlocks. The format for this change is to add one byte to the end of the brlock.tdb record with value 1 if we have level2 oplocks around. Thus this patch effectively reverts 8f41142 which I discovered while writing this change. We now legally have unaligned records. We can certainly talk about the format, but I'm not yet convinced we need an idl for this yet. This is a potentially very hot code path, and ndr marshalling has a cost. Signed-off-by: Volker Lendecke Reviewed-by: Jeremy Allison --- diff --git a/source3/locking/brlock.c b/source3/locking/brlock.c index 0d45501453..67b1701919 100644 --- a/source3/locking/brlock.c +++ b/source3/locking/brlock.c @@ -47,6 +47,7 @@ struct byte_range_lock { struct files_struct *fsp; unsigned int num_locks; bool modified; + bool have_read_oplocks; struct lock_struct *lock_data; struct db_record *record; }; @@ -81,6 +82,19 @@ struct files_struct *brl_fsp(struct byte_range_lock *brl) return brl->fsp; } +bool brl_have_read_oplocks(const struct byte_range_lock *brl) +{ + return brl->have_read_oplocks; +} + +void brl_set_have_read_oplocks(struct byte_range_lock *brl, + bool have_read_oplocks) +{ + SMB_ASSERT(brl->record != NULL); /* otherwise we're readonly */ + brl->have_read_oplocks = have_read_oplocks; + brl->modified = true; +} + /**************************************************************************** See if two locking contexts are equal. ****************************************************************************/ @@ -1878,11 +1892,18 @@ int brl_forall(void (*fn)(struct file_id id, struct server_id pid, static void byte_range_lock_flush(struct byte_range_lock *br_lck) { + size_t data_len; if (!br_lck->modified) { goto done; } - if (br_lck->num_locks == 0) { + data_len = br_lck->num_locks * sizeof(struct lock_struct); + + if (br_lck->have_read_oplocks) { + data_len += 1; + } + + if (data_len == 0) { /* No locks - delete this entry. */ NTSTATUS status = dbwrap_record_delete(br_lck->record); if (!NT_STATUS_IS_OK(status)) { @@ -1894,10 +1915,19 @@ static void byte_range_lock_flush(struct byte_range_lock *br_lck) TDB_DATA data; NTSTATUS status; - data.dptr = (uint8 *)br_lck->lock_data; - data.dsize = br_lck->num_locks * sizeof(struct lock_struct); + data.dsize = data_len; + data.dptr = talloc_array(talloc_tos(), uint8_t, data_len); + SMB_ASSERT(data.dptr != NULL); + + memcpy(data.dptr, br_lck->lock_data, + br_lck->num_locks * sizeof(struct lock_struct)); + + if (br_lck->have_read_oplocks) { + data.dptr[data_len-1] = 1; + } status = dbwrap_record_store(br_lck->record, data, TDB_REPLACE); + TALLOC_FREE(data.dptr); if (!NT_STATUS_IS_OK(status)) { DEBUG(0, ("store returned %s\n", nt_errstr(status))); smb_panic("Could not store byte range mode entry"); @@ -1932,6 +1962,7 @@ struct byte_range_lock *brl_get_locks(TALLOC_CTX *mem_ctx, files_struct *fsp) br_lck->fsp = fsp; br_lck->num_locks = 0; + br_lck->have_read_oplocks = false; br_lck->modified = False; key.dptr = (uint8 *)&fsp->file_id; @@ -1947,12 +1978,6 @@ struct byte_range_lock *brl_get_locks(TALLOC_CTX *mem_ctx, files_struct *fsp) data = dbwrap_record_get_value(br_lck->record); - if ((data.dsize % sizeof(struct lock_struct)) != 0) { - DEBUG(3, ("Got invalid brlock data\n")); - TALLOC_FREE(br_lck); - return NULL; - } - br_lck->lock_data = NULL; talloc_set_destructor(br_lck, byte_range_lock_destructor); @@ -1968,7 +1993,12 @@ struct byte_range_lock *brl_get_locks(TALLOC_CTX *mem_ctx, files_struct *fsp) return NULL; } - memcpy(br_lck->lock_data, data.dptr, data.dsize); + memcpy(br_lck->lock_data, data.dptr, + talloc_get_size(br_lck->lock_data)); + } + + if ((data.dsize % sizeof(struct lock_struct)) == 1) { + br_lck->have_read_oplocks = (data.dptr[data.dsize-1] == 1); } if (!fsp->lockdb_clean) { @@ -2038,6 +2068,10 @@ static void brl_get_locks_readonly_parser(TDB_DATA key, TDB_DATA data, br_lock, data.dptr, data.dsize); br_lock->num_locks = data.dsize / sizeof(struct lock_struct); + if ((data.dsize % sizeof(struct lock_struct)) == 1) { + br_lock->have_read_oplocks = (data.dptr[data.dsize-1] == 1); + } + *state->br_lock = br_lock; } @@ -2079,6 +2113,7 @@ struct byte_range_lock *brl_get_locks_readonly(files_struct *fsp) if (br_lock == NULL) { goto fail; } + br_lock->have_read_oplocks = rw->have_read_oplocks; br_lock->num_locks = rw->num_locks; br_lock->lock_data = (struct lock_struct *)talloc_memdup( br_lock, rw->lock_data, lock_data_size); diff --git a/source3/locking/proto.h b/source3/locking/proto.h index 02e2bf5307..f6ae462bac 100644 --- a/source3/locking/proto.h +++ b/source3/locking/proto.h @@ -30,6 +30,9 @@ void brl_shutdown(void); unsigned int brl_num_locks(const struct byte_range_lock *brl); struct files_struct *brl_fsp(struct byte_range_lock *brl); +bool brl_have_read_oplocks(const struct byte_range_lock *brl); +void brl_set_have_read_oplocks(struct byte_range_lock *brl, + bool have_read_oplocks); NTSTATUS brl_lock_windows_default(struct byte_range_lock *br_lck, struct lock_struct *plock,