samba 3.6.0rc2 question
Jeremy Allison
jra at samba.org
Fri Jul 15 13:22:38 MDT 2011
On Thu, Jul 14, 2011 at 05:42:26PM -0700, Jeremy Allison wrote:
> On Wed, Jul 13, 2011 at 03:34:58PM -0700, Herb Lewis wrote:
> > I'm trying to use the vfs functions for brl_lock_windows and
> > brl_unlock_windows however it doesn't seem like the brl_unlock_windows
> > function is getting called on file close unless posiz locks is set.
> >
> > locking_close_file is called which in turn calls brl_close_fnum but
> > this only calls brl_unlock which is what calls SMB_VFS_BRL_UNLOCK_WINDOWS
> > within an if(lp_posix_locking(fsp->conn->params)) clause.
> >
> > This seems to be a bug as the vfs unlock routine will never get called
> > to release the locks on close. I'm not quite sure where this fix needs
> > to go though.
>
> Oh - I see the problem ! We're getting bitten by an optimization
> bug which would only bite if you've hooked into the SMB_VFS_BRL_UNLOCK_WINDOWS
> call.
>
> The code inside brl_close_fnum() is taking a shortcut if we are not
> mapping onto POSIX locks inside smbd.
>
> In that case it "knows" that locks are only stored in the tdb mapped
> into memory and so bulk deletes them from the copy of this array
> stored on the br_lck struct and re-stores them - WITHOUT calling
> the VFS call on each unlock.
>
> It might be that we can just remove this optimization completely.
> Give me a little while to create a patch that does this that you
> can use. I'll investagate what really needs to get fixed here.
Here is a version (for v3-6-test) that just completely removes all the optimization
and calls brl_unlock() for every lock on file close. Passes all tests
and seems to be the thing to push for master.
If you can log a bug on this I'll get it scheduled for 3.6.1 (it's
too late for 3.6.0 I'm afraid).
It really does seem that premature optimization is the root of all
evil :-).
Cheers,
Jeremy.
-------------- next part --------------
diff --git a/source3/locking/brlock.c b/source3/locking/brlock.c
index c325338..8b8605d 100644
--- a/source3/locking/brlock.c
+++ b/source3/locking/brlock.c
@@ -1485,137 +1485,38 @@ void brl_close_fnum(struct messaging_context *msg_ctx,
files_struct *fsp = br_lck->fsp;
uint16 tid = fsp->conn->cnum;
int fnum = fsp->fnum;
- unsigned int i, j, dcount=0;
- int num_deleted_windows_locks = 0;
+ unsigned int i;
struct lock_struct *locks = br_lck->lock_data;
struct server_id pid = sconn_server_id(fsp->conn->sconn);
- bool unlock_individually = False;
- bool posix_level2_contention_ended = false;
-
- if(lp_posix_locking(fsp->conn->params)) {
-
- /* Check if there are any Windows locks associated with this dev/ino
- pair that are not this fnum. If so we need to call unlock on each
- one in order to release the system POSIX locks correctly. */
-
- for (i=0; i < br_lck->num_locks; i++) {
- struct lock_struct *lock = &locks[i];
-
- if (!procid_equal(&lock->context.pid, &pid)) {
- continue;
+ struct lock_struct *locks_copy;
+ unsigned int num_locks_copy;
+
+ /* Copy the current lock array. */
+ if (br_lck->num_locks) {
+ locks_copy = (struct lock_struct *)TALLOC_MEMDUP(br_lck, locks, br_lck->num_locks * sizeof(struct lock_struct));
+ if (!locks_copy) {
+ smb_panic("brl_close_fnum: talloc failed");
}
-
- if (lock->lock_type != READ_LOCK && lock->lock_type != WRITE_LOCK) {
- continue; /* Ignore pending. */
- }
-
- if (lock->context.tid != tid || lock->fnum != fnum) {
- unlock_individually = True;
- break;
- }
- }
-
- if (unlock_individually) {
- struct lock_struct *locks_copy;
- unsigned int num_locks_copy;
-
- /* Copy the current lock array. */
- if (br_lck->num_locks) {
- locks_copy = (struct lock_struct *)TALLOC_MEMDUP(br_lck, locks, br_lck->num_locks * sizeof(struct lock_struct));
- if (!locks_copy) {
- smb_panic("brl_close_fnum: talloc failed");
- }
- } else {
- locks_copy = NULL;
- }
-
- num_locks_copy = br_lck->num_locks;
-
- for (i=0; i < num_locks_copy; i++) {
- struct lock_struct *lock = &locks_copy[i];
-
- if (lock->context.tid == tid && procid_equal(&lock->context.pid, &pid) &&
- (lock->fnum == fnum)) {
- brl_unlock(msg_ctx,
- br_lck,
- lock->context.smblctx,
- pid,
- lock->start,
- lock->size,
- lock->lock_flav);
- }
- }
- return;
- }
+ } else {
+ locks_copy = NULL;
}
- /* We can bulk delete - any POSIX locks will be removed when the fd closes. */
+ num_locks_copy = br_lck->num_locks;
- /* Remove any existing locks for this fnum (or any fnum if they're POSIX). */
+ for (i=0; i < num_locks_copy; i++) {
+ struct lock_struct *lock = &locks_copy[i];
- for (i=0; i < br_lck->num_locks; i++) {
- struct lock_struct *lock = &locks[i];
- bool del_this_lock = False;
-
- if (lock->context.tid == tid && procid_equal(&lock->context.pid, &pid)) {
- if ((lock->lock_flav == WINDOWS_LOCK) && (lock->fnum == fnum)) {
- del_this_lock = True;
- num_deleted_windows_locks++;
- contend_level2_oplocks_end(br_lck->fsp,
- LEVEL2_CONTEND_WINDOWS_BRL);
- } else if (lock->lock_flav == POSIX_LOCK) {
- del_this_lock = True;
-
- /* Only end level2 contention once for posix */
- if (!posix_level2_contention_ended) {
- posix_level2_contention_ended = true;
- contend_level2_oplocks_end(br_lck->fsp,
- LEVEL2_CONTEND_POSIX_BRL);
- }
- }
- }
-
- if (del_this_lock) {
- /* Send unlock messages to any pending waiters that overlap. */
- for (j=0; j < br_lck->num_locks; j++) {
- struct lock_struct *pend_lock = &locks[j];
-
- /* Ignore our own or non-pending locks. */
- if (!IS_PENDING_LOCK(pend_lock->lock_type)) {
- continue;
- }
-
- /* Optimisation - don't send to this fnum as we're
- closing it. */
- if (pend_lock->context.tid == tid &&
- procid_equal(&pend_lock->context.pid, &pid) &&
- pend_lock->fnum == fnum) {
- continue;
- }
-
- /* We could send specific lock info here... */
- if (brl_pending_overlap(lock, pend_lock)) {
- messaging_send(msg_ctx, pend_lock->context.pid,
- MSG_SMB_UNLOCK, &data_blob_null);
- }
- }
-
- /* found it - delete it */
- if (br_lck->num_locks > 1 && i < br_lck->num_locks - 1) {
- memmove(&locks[i], &locks[i+1],
- sizeof(*locks)*((br_lck->num_locks-1) - i));
- }
- br_lck->num_locks--;
- br_lck->modified = True;
- i--;
- dcount++;
+ if (lock->context.tid == tid && procid_equal(&lock->context.pid, &pid) &&
+ (lock->fnum == fnum)) {
+ brl_unlock(msg_ctx,
+ br_lck,
+ lock->context.smblctx,
+ pid,
+ lock->start,
+ lock->size,
+ lock->lock_flav);
}
}
-
- if(lp_posix_locking(fsp->conn->params) && num_deleted_windows_locks) {
- /* Reduce the Windows lock POSIX reference count on this dev/ino pair. */
- reduce_windows_lock_ref_count(fsp, num_deleted_windows_locks);
- }
}
/****************************************************************************
More information about the samba-technical
mailing list