Re: [PATCH 04/11] ext4: completed_io locking cleanup V4

On Tue 02-10-12 11:16:38, Dmitry Monakhov wrote:
...

quoted

quoted

+static int ext4_do_flush_completed_IO(struct inode *inode,
+	while (!list_empty(&unwritten)) {
+		io = list_entry(unwritten.next, ext4_io_end_t, list);
+		BUG_ON(!(io->flag & EXT4_IO_END_UNWRITTEN));
+		list_del_init(&io->list);
+
+		err = ext4_end_io(io);
+		if (unlikely(!ret && err))
+			ret = err;
+
+		list_add_tail(&io->list, &complete);
+	}
+	/* It is important to update all flags for all end_io in one shot w/o
+	 * dropping the lock.*/

  Why? It would seem that once io structures are moved from
i_completed_io_list, they are unreachable by anyone else?

  You seem to have missed this comment?

quoted

quoted

+	spin_lock_irqsave(&ei->i_completed_io_lock, flags);
+	while (!list_empty(&complete)) {
+		io = list_entry(complete.next, ext4_io_end_t, list);
+		io->flag &= ~EXT4_IO_END_UNWRITTEN;
+		/* end_io context can not be destroyed now because it still
+		 * used by queued worker. Worker thread will destroy it later */
+		if (io->flag & EXT4_IO_END_QUEUED)
+			list_del_init(&io->list);
+		else
+			list_move(&io->list, &to_free);
+	}
+	/* If we are called from worker context, it is time to clear queued
+	 * flag, and destroy it's end_io if it was converted already */
+	if (work_io) {
+		work_io->flag &= ~EXT4_IO_END_QUEUED;
+		if (!(work_io->flag & EXT4_IO_END_UNWRITTEN))
+			list_add_tail(&work_io->list, &to_free);
 	}
-	list_del_init(&io->list);
 	spin_unlock_irqrestore(&ei->i_completed_io_lock, flags);
-	(void) ext4_end_io_nolock(io);
-	mutex_unlock(&inode->i_mutex);
-free:
-	ext4_free_io_end(io);
+
+	while (!list_empty(&to_free)) {
+		io = list_entry(to_free.next, ext4_io_end_t, list);
+		list_del_init(&io->list);
+		ext4_free_io_end(io);
+	}
+	return ret;
+}
+
+/*
+ * work on completed aio dio IO, to convert unwritten extents to extents
+ */
+static void ext4_end_io_work(struct work_struct *work)
+{
+	ext4_io_end_t *io = container_of(work, ext4_io_end_t, work);
+	ext4_do_flush_completed_IO(io->inode, io);
+}
+
+int ext4_flush_completed_IO(struct inode *inode)
+{
+	return ext4_do_flush_completed_IO(inode, NULL);
 }

  Also it seems that when ext4_flush_completed_IO() is called, workqueue
can have several IO structures queued in its local lists thus we miss them
here and don't properly wait for all conversions?

No it is not. Because list drained atomically, and 
add_complete_io will queue work only if list is empty.

Race between conversion and dequeue-process is not possible because
we hold lock during entire walk of complete_list, so from external
point of view we mark list as conversed(clear unwritten flag)
happens atomically. I've drawn all possible situations and race not
happen. If you know any please let me know.

  I guess I'm missing something obvious. So let's go step by step:
1) ext4_flush_completed_IO() must make sure there is no outstanding
   conversion for the inode.
2) Now assume we have non-empty i_completed_io_list - thus work is queued.
3) The following situation seems to be possible:

CPU1					CPU2
(worker thread)				(truncate)
ext4_end_io_work()
  ext4_do_flush_completed_IO()
    spin_lock_irqsave(&ei->i_completed_io_lock, flags);
    dump_completed_IO(inode);
    list_replace_init(&ei->i_completed_io_list, &unwritten);
    spin_unlock_irqrestore(&ei->i_completed_io_lock, flags);

					ext4_flush_completed_IO()
					  ext4_do_flush_completed_IO()
					    - sees empty i_completed_io_list
					     => exits

  But we still have some conversions pending in 'unwritten' list. What am
I missing?

								Honza