Andrew, thanks for the review.
On Wed, Jan 23, 2008 at 02:05:09PM -0800, Andrew Morton wrote:
> > On Thu, 17 Jan 2008 14:35:29 -0800 Mark Fasheh <[EMAIL PROTECTED]> wrote:
> > The node maps that are set/unset by these votes are no longer relevant, thus
> > we can remove the mount and umount votes. Since those are the last two
> > remaining votes, we can also remove the entire vote infrastructure.
> >
> > The vote thread has been renamed to the downconvert thread, and the small
> > amount of functionality related to managing it has been moved into
> > fs/ocfs2/dlmglue.c. All references to votes have been removed or updated.
> >
>
> Locking looks fishy.
Btw, the downconvert code is functionally identical from what was in vote.c
for years now. It just got moved into dlmglue.c and renamed.
> > +static void ocfs2_downconvert_thread_do_work(struct ocfs2_super *osb)
> > +{
> > + unsigned long processed;
> > + struct ocfs2_lock_res *lockres;
> > +
> > + mlog_entry_void();
> > +
> > + spin_lock(&osb->dc_task_lock);
> > + /* grab this early so we know to try again if a state change and
> > + * wake happens part-way through our work */
> > + osb->dc_work_sequence = osb->dc_wake_sequence;
> > +
> > + processed = osb->blocked_lock_count;
> > + while (processed) {
> > + BUG_ON(list_empty(&osb->blocked_lock_list));
> > +
> > + lockres = list_entry(osb->blocked_lock_list.next,
> > + struct ocfs2_lock_res, l_blocked_list);
> > + list_del_init(&lockres->l_blocked_list);
> > + osb->blocked_lock_count--;
> > + spin_unlock(&osb->dc_task_lock);
> > +
> > + BUG_ON(!processed);
> > + processed--;
> > +
> > + ocfs2_process_blocked_lock(osb, lockres);
> > +
> > + spin_lock(&osb->dc_task_lock);
> > + }
> > + spin_unlock(&osb->dc_task_lock);
>
> Once the lock has been dropped there is (apparently) nothing to prevent
> alteration of the list and of ->blocked_lock_count. If this happens,
> either items will be missed or we go BUG.
The rule is that only the downconvert thread is allowed to remove locks from
that list. Everyone else just decides a lock needs downconverting and queues
it.
Attached to this e-mail is a patch to better document this.
> > + mlog_exit_void();
> > +}
> > +
> > +static int ocfs2_downconvert_thread_lists_empty(struct ocfs2_super *osb)
> > +{
> > + int empty = 0;
> > +
> > + spin_lock(&osb->dc_task_lock);
> > + if (list_empty(&osb->blocked_lock_list))
> > + empty = 1;
> > +
> > + spin_unlock(&osb->dc_task_lock);
> > + return empty;
> > +}
>
> This function appears to be returning a value which is unreliable once the
> lock was dropped.
In this case that's ok. It's only used during dlmglue shutdown when no new
locks should ever be queued.
> > +static int ocfs2_downconvert_thread_should_wake(struct ocfs2_super *osb)
> > +{
> > + int should_wake = 0;
> > +
> > + spin_lock(&osb->dc_task_lock);
> > + if (osb->dc_work_sequence != osb->dc_wake_sequence)
> > + should_wake = 1;
> > + spin_unlock(&osb->dc_task_lock);
> > +
> > + return should_wake;
> > +}
>
> Ditto.
Another case where it's ok :)
> > +int ocfs2_downconvert_thread(void *arg)
> > +{
> > + int status = 0;
> > + struct ocfs2_super *osb = arg;
> > +
> > + /* only quit once we've been asked to stop and there is no more
> > + * work available */
> > + while (!(kthread_should_stop() &&
> > + ocfs2_downconvert_thread_lists_empty(osb))) {
>
> Extra whitespace
Ok, a cleanup of this and the other whitespace problems you pointed out (in
resize.c) have been commited to the tree.
--Mark
--
Mark Fasheh
Principal Software Developer, Oracle
[EMAIL PROTECTED]
From: Mark Fasheh <[EMAIL PROTECTED]>
ocfs2: document access rules for blocked_lock_list
ocfs2_super->blocked_lock_list and ocfs2_super->blocked_lock_count have some
usage restrictions which aren't immediately obvious to anyone reading the
code. It's a good idea to document this so that we avoid making costly
mistakes in the future.
Signed-off-by: Mark Fasheh <[EMAIL PROTECTED]>
---
fs/ocfs2/ocfs2.h | 6 ++++++
1 files changed, 6 insertions(+), 0 deletions(-)
diff --git a/fs/ocfs2/ocfs2.h b/fs/ocfs2/ocfs2.h
index 22e334d..d084805 100644
--- a/fs/ocfs2/ocfs2.h
+++ b/fs/ocfs2/ocfs2.h
@@ -262,6 +262,12 @@ struct ocfs2_super
unsigned long dc_wake_sequence;
unsigned long dc_work_sequence;
+ /*
+ * Any thread can add locks to the list, but the downconvert
+ * thread is the only one allowed to remove locks. Any change
+ * to this rule requires updating
+ * ocfs2_downconvert_thread_do_work().
+ */
struct list_head blocked_lock_list;
unsigned long blocked_lock_count;
--
1.5.3.6
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
