[PATCH/RFC] Simplify implementation of lock / unlock operations in FSFS

[Sergey Raevskiy <se...@visualsvn.com>]

Hi!

Recently I've spent some time investigating how lock and unlock are implemented
in FSFS and can suggest the following simplification.  I attempted just to make
this part clearer and a new code is intended to behave exactly the same.

The patch is attached.

Log message:
[[[
Simplify implementation of svn_fs_fs__lock() / svn_fs_fs__unlock().

* subversion/libsvn_fs_fs/lock.c
  (schedule_index_update): New helper function.
  (struct lock_info_t,
   struct unlock_info_t): Drop the unused fields.
  (lock_body,
   unlock_body): Rework the algorithm.
]]]

PS. I've noticed at least one improper pool usage in locking code:
[[[
info->lock = svn_lock_create(lb->result_pool);

...

info->lock->path = info->path;
info->lock->owner = lb->fs->access_ctx->username;
info->lock->comment = lb->comment;
info->lock->is_dav_comment = lb->is_dav_comment;
info->lock->creation_date = apr_time_now();
info->lock->expiration_date = lb->expiration_date;
]]]

The svn_lock_t * object is created in the result_pool, but its fields like PATH
and OWNER are not getting apr_pstrdup()'ed.  I'm going to fix this in another
patch.

Re: [PATCH/RFC] Simplify implementation of lock / unlock operations in FSFS

[Philip Martin <ph...@wandisco.com>]

Sergey Raevskiy <se...@visualsvn.com> writes:

> Yes, the behaviour is *not* exactly the same -- the index files is updated in
> different order.  Furthermore, I've realized that current version of algorithm
> will set some locks even if update of some index files is failed.

I think that is OK.  What matters is that the lock database remains
valid when writes fail: i.e. lock files never exist without all the
index entries.

Committed in r1659212.  Thanks!

-- 
Philip Martin | Subversion Committer
WANdisco // *Non-Stop Data*

Re: [PATCH/RFC] Simplify implementation of lock / unlock operations in FSFS

[Sergey Raevskiy <se...@visualsvn.com>]

>> Recently I've spent some time investigating how lock and unlock are
>> implemented in FSFS and can suggest the following simplification.  I
>> attempted just to make this part clearer and a new code is intended to
>> behave exactly the same.
>
> It's not exactly the same, index files are written/deleted in a
> different order.  I think that change is OK but can you confirm that
> this is expected?

Yes, the behaviour is *not* exactly the same -- the index files is updated in
different order.  Furthermore, I've realized that current version of algorithm
will set some locks even if update of some index files is failed.

Let's say we need to lock the following paths:

    /abc
    /foo/bar

Even if updating of index file for '/foo' will fail, the lock for '/abc' will
be set.  I don't know if it really matters since this depends on 'implementation
details' like sorting order etc.  If we really need to follow this 'set as much
locks as much as possible' strategy then my patch is not correct.  BTW, current
version is not following this strategy strictly.

Re: [PATCH/RFC] Simplify implementation of lock / unlock operations in FSFS

[Philip Martin <ph...@wandisco.com>]

Sergey Raevskiy <se...@visualsvn.com> writes:

> Recently I've spent some time investigating how lock and unlock are
> implemented in FSFS and can suggest the following simplification.  I
> attempted just to make this part clearer and a new code is intended to
> behave exactly the same.

It's not exactly the same, index files are written/deleted in a
different order.  I think that change is OK but can you confirm that
this is expected?

-- 
Philip Martin | Subversion Committer
WANdisco // *Non-Stop Data*