On Saturday January 12, [email protected] wrote:
> This is a combined patch that has:
>
> * changes made by Christoph Hellwig
> * code segment that handles f_locks so we would not walk inode->i_flock
> list twice.
>
> If agreed, please re-add your "ack-by" and "signed-off" lines
> respectively. Thanks ...
>
> - int i, ret = 0;
> + int i, ret = 0, inspect_file;
>
> mutex_lock(&nlm_file_mutex);
> for (i = 0; i < FILE_NRHASH; i++) {
> hlist_for_each_entry_safe(file, pos, next, &nlm_files[i], f_list) {
> file->f_count++;
> mutex_unlock(&nlm_file_mutex);
> + inspect_file = 1;
>
> /* Traverse locks, blocks and shares of this file
> * and update file->f_locks count */
> - if (nlm_inspect_file(host, file, match))
> +
> + if (unlikely(failover)) {
> + if (!failover(data, file)) {
> + inspect_file = 0;
> + file->f_locks = nlm_file_inuse(file);
> + }
> + }
> +
> + if (inspect_file && nlm_inspect_file(data, file, match))
> ret = 1;
if (unlikely(failover) &&
!failover(data, file))
file->f_locks = nlm_file_inuse(file);
else if (nlm_inspect_file(data, file, match))
ret = 1;
Though the logic still isn't very clear... maybe:
if (likely(failover == NULL) ||
failover(data, file))
ret |= nlm_inspect_file(data, file, match);
else
file->f_locks = nlm_file_inuse(file);
Actually I would like to make nlm_inspect_file return 'void'.
The returned value of '1' is ultimately either ignored or it triggers
a BUG(). And the case where it triggers a BUG is the "host != NULL"
case. (I think - if someone could check, that would be good).
So putting BUG_ON(host) in nlm_traverse_locks (along with a nice big
comment) would mean we can discard the return value from
nlm_traverse_locks and nlm_inspect_file and nlm_traverse_files.
Also, if we could change the function name 'failover' to some sort of
verb like "is_failover" or "is_failover_file", then the above could be
if (likely(is_failover_file == NULL) ||
is_failover_file(data, file))
/* note nlm_inspect_file updates f_locks */
nlm_inspect_file(data, file, match);
else
file->f_locks = nlm_file_inuse(file);
>
> mutex_lock(&nlm_file_mutex);
> file->f_count--;
> /* No more references to this file. Let go of it. */
> - if (list_empty(&file->f_blocks) && !file->f_locks
> + if (!file->f_locks && list_empty(&file->f_blocks)
Is this change actually achieving something? or is it just noise?
NeilBrown
Neil Brown wrote:
> On Saturday January 12, [email protected] wrote:
>
>> This is a combined patch that has:
>>
>> * changes made by Christoph Hellwig
>> * code segment that handles f_locks so we would not walk inode->i_flock
>> list twice.
>>
>>
.....
>
> if (unlikely(failover) &&
> !failover(data, file))
> file->f_locks = nlm_file_inuse(file);
> else if (nlm_inspect_file(data, file, match))
> ret = 1;
>
> Though the logic still isn't very clear... maybe:
>
> if (likely(failover == NULL) ||
> failover(data, file))
> ret |= nlm_inspect_file(data, file, match);
> else
> file->f_locks = nlm_file_inuse(file);
>
> Actually I would like to make nlm_inspect_file return 'void'.
> The returned value of '1' is ultimately either ignored or it triggers
> a BUG(). And the case where it triggers a BUG is the "host != NULL"
> case. (I think - if someone could check, that would be good).
> So putting BUG_ON(host) in nlm_traverse_locks (along with a nice big
> comment) would mean we can discard the return value from
> nlm_traverse_locks and nlm_inspect_file and nlm_traverse_files.
>
Current logic BUG() when:
1. host is not NULL; and
2. nlm_traverse_locks() somehow can't unlock the file.
I don't feel comfortable to change the existing code structure,
especially a BUG() statement. It would be better to separate lock
failover function away from lockd code clean-up. This is to make it
easier for problem isolations (just in case).
On the other hand, if we view "ret" as a file count that tells us how
many files fail to get unlocked, it would be great for debugging
purpose. So the changes I made (currently in the middle of cluster
testing) end up like this:
if (likely(is_failover_file == NULL) ||
is_failover_file(data, file)) {
/*
* Note that nlm_inspect_file updates f_locks
* and ret is the number of files that can't
* be unlocked.
*/
ret += nlm_inspect_file(data, file, match);
} else
file->f_locks = nlm_file_inuse(file);
>
>
>
>
>>
>> mutex_lock(&nlm_file_mutex);
>> file->f_count--;
>> /* No more references to this file. Let go of it. */
>> - if (list_empty(&file->f_blocks) && !file->f_locks
>> + if (!file->f_locks && list_empty(&file->f_blocks)
>>
>
> Is this change actually achieving something? or is it just noise?
>
Not really - but I thought checking for f_locks would be faster (tiny
bit of optimization :))
-- Wendy
>
> NeilBrown
>
>