[RFC 0/5] dio: clean up completion phase of direct_io_worker()

[RFC 0/5] dio: clean up completion phase of direct_io_worker()

[Zach Brown]

dio: clean up completion phase of direct_io_worker()

There have been a lot of bugs recently due to the way direct_io_worker() tries
to decide how to finish direct IO operations.  In the worst examples it has
failed to call aio_complete() at all (hang) or called it too many times (oops).

This set of patches cleans up the completion phase with the goal of removing
the complexity that lead to these bugs.  We end up with one path that
calculates the result of the operation after all off the bios have completed.
We decide when to generate a result of the operation using that path based on
the final release of a refcount on the dio structure.

I tried to progress towards the final state in steps that were relatively easy
to understand.  Each step should compile but I only tested the final result of
having all the patches applied.

The patches result in a slight net decrease in code and binary size:

 2.6.18-rc4-dio-cleanup/fs/direct-io.c |    8
 2.6.18-rc5-dio-cleanup/fs/direct-io.c |   94 +++++------
 2.6.18-rc5-dio-cleanup/mm/filemap.c   |    4
 fs/direct-io.c                        |  273 ++++++++++++++--------------------
 4 files changed, 159 insertions(+), 220 deletions(-)

   text    data     bss     dec     hex filename
2592385  450996  210296 3253677  31a5ad vmlinux.before
2592113  450980  210296 3253389  31a48d vmlinux.after

The patches pass light testing with aio-stress, the direct IO tests I could
manage to get running in LTP, and some home-brew functional tests.  It's still
making its way through stress testing.  It should not be merged until we hear
from that more rigorous testing, I don't think.

I hoped to get some feedback (and maybe volunteers for testing!) by sending the
patches out before waiting for the stress tests.

- z

[RFC 1/5] dio: centralize completion in dio_complete()

[Zach Brown]

dio: centralize completion in dio_complete()

The mechanics which decide the result of a direct IO operation were duplicated
in the sync and async paths.

The async path didn't check page_errors which can manifest as silently
returning success when the final pointer in an operation faults and its
matching file region is filled with zeros.

The sync path and async path differed in whether they passed errors to the
caller's dio->end_io operation.  The async path was passing errors to it which
trips an assertion in XFS, though it is apparently harmless.

This centralizes the completion phase of dio ops in one place.  AIO will now
return EFAULT consistently and all paths fall back to the previously sync
behaviour of passing the number of bytes 'transferred' to the dio->end_io
callback, regardless of errors.

dio_await_completion() doesn't have to propogate EIO from non-uptodate
bios now that it's being propogated through dio_complete() via dio->io_error.
This lets it return void which simplifies its sole caller.

Signed-off-by: Zach Brown <zach.brown@oracle.com>
---

 fs/direct-io.c |   94 +++++++++++++++++++++--------------------------
 1 file changed, 42 insertions(+), 52 deletions(-)

Index: 2.6.18-rc6-dio-cleanup/fs/direct-io.c
===================================================================
--- 2.6.18-rc6-dio-cleanup.orig/fs/direct-io.c
+++ 2.6.18-rc6-dio-cleanup/fs/direct-io.c
@@ -209,19 +209,46 @@ static struct page *dio_get_page(struct 
 	return dio->pages[dio->head++];
 }
 
-/*
- * Called when all DIO BIO I/O has been completed - let the filesystem
- * know, if it registered an interest earlier via get_block.  Pass the
- * private field of the map buffer_head so that filesystems can use it
- * to hold additional state between get_block calls and dio_complete.
+/**
+ * dio_complete() - called when all DIO BIO I/O has been completed
+ * @offset: the byte offset in the file of the completed operation
+ *
+ * This releases locks as dictated by the locking type, lets interested parties
+ * know that a DIO operation has completed, and calculates the resulting return
+ * code for the operation.
+ *
+ * It lets the filesystem know if it registered an interest earlier via
+ * get_block.  Pass the private field of the map buffer_head so that
+ * filesystems can use it to hold additional state between get_block calls and
+ * dio_complete.
  */
-static void dio_complete(struct dio *dio, loff_t offset, ssize_t bytes)
+static int dio_complete(struct dio *dio, loff_t offset, int ret)
 {
+	ssize_t transferred = 0;
+
+	if (dio->result) {
+		transferred = dio->result;
+
+		/* Check for short read case */
+		if ((dio->rw == READ) && ((offset + transferred) > dio->i_size))
+			transferred = dio->i_size - offset;
+	}
+
 	if (dio->end_io && dio->result)
-		dio->end_io(dio->iocb, offset, bytes, dio->map_bh.b_private);
+		dio->end_io(dio->iocb, offset, transferred,
+			    dio->map_bh.b_private);
 	if (dio->lock_type == DIO_LOCKING)
 		/* lockdep: non-owner release */
 		up_read_non_owner(&dio->inode->i_alloc_sem);
+
+	if (ret == 0)
+		ret = dio->page_errors;
+	if (ret == 0)
+		ret = dio->io_error;
+	if (ret == 0)
+		ret = transferred;
+
+	return ret;
 }
 
 /*
@@ -235,8 +262,7 @@ static void finished_one_bio(struct dio 
 	spin_lock_irqsave(&dio->bio_lock, flags);
 	if (dio->bio_count == 1) {
 		if (dio->is_async) {
-			ssize_t transferred;
-			loff_t offset;
+			int ret;
 
 			/*
 			 * Last reference to the dio is going away.
@@ -244,24 +270,12 @@ static void finished_one_bio(struct dio 
 			 */
 			spin_unlock_irqrestore(&dio->bio_lock, flags);
 
-			/* Check for short read case */
-			transferred = dio->result;
-			offset = dio->iocb->ki_pos;
-
-			if ((dio->rw == READ) &&
-			    ((offset + transferred) > dio->i_size))
-				transferred = dio->i_size - offset;
-
-			/* check for error in completion path */
-			if (dio->io_error)
-				transferred = dio->io_error;
-
-			dio_complete(dio, offset, transferred);
+			ret = dio_complete(dio, dio->iocb->ki_pos, 0);
 
 			/* Complete AIO later if falling back to buffered i/o */
 			if (dio->result == dio->size ||
 				((dio->rw == READ) && dio->result)) {
-				aio_complete(dio->iocb, transferred, 0);
+				aio_complete(dio->iocb, ret, 0);
 				kfree(dio);
 				return;
 			} else {
@@ -433,10 +447,8 @@ static int dio_bio_complete(struct dio *
 /*
  * Wait on and process all in-flight BIOs.
  */
-static int dio_await_completion(struct dio *dio)
+static void dio_await_completion(struct dio *dio)
 {
-	int ret = 0;
-
 	if (dio->bio)
 		dio_bio_submit(dio);
 
@@ -447,13 +459,9 @@ static int dio_await_completion(struct d
 	 */
 	while (dio->bio_count) {
 		struct bio *bio = dio_await_one(dio);
-		int ret2;
-
-		ret2 = dio_bio_complete(dio, bio);
-		if (ret == 0)
-			ret = ret2;
+		/* io errors are propogated through dio->io_error */
+		dio_bio_complete(dio, bio);
 	}
-	return ret;
 }
 
 /*
@@ -1119,28 +1127,10 @@ direct_io_worker(int rw, struct kiocb *i
 			kfree(dio);
 		}
 	} else {
-		ssize_t transferred = 0;
-
 		finished_one_bio(dio);
-		ret2 = dio_await_completion(dio);
-		if (ret == 0)
-			ret = ret2;
-		if (ret == 0)
-			ret = dio->page_errors;
-		if (dio->result) {
-			loff_t i_size = i_size_read(inode);
+		dio_await_completion(dio);
 
-			transferred = dio->result;
-			/*
-			 * Adjust the return value if the read crossed a
-			 * non-block-aligned EOF.
-			 */
-			if (rw == READ && (offset + transferred > i_size))
-				transferred = i_size - offset;
-		}
-		dio_complete(dio, offset, transferred);
-		if (ret == 0)
-			ret = transferred;
+		ret = dio_complete(dio, offset, ret);
 
 		/* We could have also come here on an AIO file extend */
 		if (!is_sync_kiocb(iocb) && (rw & WRITE) &&

[RFC 2/5] dio: call blk_run_address_space() once per op

[Zach Brown]

dio: call blk_run_address_space() once per op

We only need to call blk_run_address_space() once after all the bios for the
direct IO op have been submitted.  This removes the chance of calling
blk_run_address_space() after spurious wake ups as the sync path waits for bios
to drain.  It's also one less difference betwen the sync and async paths.

In the process we remove a redundant dio_bio_submit() that its caller had
already performed. 

Signed-off-by: Zach Brown <zach.brown@oracle.com>
---

 fs/direct-io.c |    8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)

Index: 2.6.18-rc6-dio-cleanup/fs/direct-io.c
===================================================================
--- 2.6.18-rc6-dio-cleanup.orig/fs/direct-io.c
+++ 2.6.18-rc6-dio-cleanup/fs/direct-io.c
@@ -403,7 +403,6 @@ static struct bio *dio_await_one(struct 
 		if (dio->bio_list == NULL) {
 			dio->waiter = current;
 			spin_unlock_irqrestore(&dio->bio_lock, flags);
-			blk_run_address_space(dio->inode->i_mapping);
 			io_schedule();
 			spin_lock_irqsave(&dio->bio_lock, flags);
 			dio->waiter = NULL;
@@ -449,9 +448,6 @@ static int dio_bio_complete(struct dio *
  */
 static void dio_await_completion(struct dio *dio)
 {
-	if (dio->bio)
-		dio_bio_submit(dio);
-
 	/*
 	 * The bio_lock is not held for the read of bio_count.
 	 * This is ok since it is the dio_bio_complete() that changes
@@ -1077,6 +1073,9 @@ direct_io_worker(int rw, struct kiocb *i
 	if (dio->bio)
 		dio_bio_submit(dio);
 
+	/* All IO is now issued, send it on its way */
+	blk_run_address_space(inode->i_mapping);
+
 	/*
 	 * It is possible that, we return short IO due to end of file.
 	 * In that case, we need to release all the pages we got hold on.
@@ -1105,7 +1104,6 @@ direct_io_worker(int rw, struct kiocb *i
 		if (ret == 0)
 			ret = dio->result;
 		finished_one_bio(dio);		/* This can free the dio */
-		blk_run_address_space(inode->i_mapping);
 		if (should_wait) {
 			unsigned long flags;
 			/*

[RFC 3/5] dio: formalize bio counters as a dio reference count

[Zach Brown]

dio: formalize bio counters as a dio reference count

Previously we had two confusing counts of bio progress.  'bio_count' was
decremented as bios were processed and freed by the dio core.  It was used to
indicate final completion of the dio operation.  'bios_in_flight' reflected how
many bios were between submit_bio() and bio->end_io.  It was used by the sync
path to decide when to wake up and finish completing bios and was ignored
by the async path.

This patch collapses the two notions into one notion of a dio reference count.
bios hold a dio reference when they're between submit_bio and bio->end_io.

Since bios_in_flight was only used in the sync path it is now equivalent
to dio->refcount - 1 which accounts for direct_io_worker() holding a 
reference for the duration of the operation.

dio_bio_complete() -> finished_one_bio() was called from the sync path after
finding bios on the list that the bio->end_io function had deposited.
finished_one_bio() can not drop the dio reference on behalf of these bios now
because bio->end_io already has.  The is_async test in finished_one_bio() meant
that it never actually did anything other than drop the bio_count for sync
callers.  So we remove its refcount decrement, don't call it from
dio_bio_complete(), and hoist its call up into the async dio_bio_complete()
caller after an explicit refcount decrement.  It is renamed dio_complete_aio()
to reflect the remaining work it actually does.

Signed-off-by: Zach Brown <zach.brown@oracle.com>
---

 fs/direct-io.c |  140 ++++++++++++++++++++++-------------------------
 1 file changed, 66 insertions(+), 74 deletions(-)

Index: 2.6.18-rc6-dio-cleanup/fs/direct-io.c
===================================================================
--- 2.6.18-rc6-dio-cleanup.orig/fs/direct-io.c
+++ 2.6.18-rc6-dio-cleanup/fs/direct-io.c
@@ -120,9 +120,8 @@ struct dio {
 	int page_errors;		/* errno from get_user_pages() */
 
 	/* BIO completion state */
+	atomic_t refcount;		/* direct_io_worker() and bios */
 	spinlock_t bio_lock;		/* protects BIO fields below */
-	int bio_count;			/* nr bios to be completed */
-	int bios_in_flight;		/* nr bios in flight */
 	struct bio *bio_list;		/* singly linked via bi_private */
 	struct task_struct *waiter;	/* waiting task (NULL if none) */
 
@@ -255,44 +254,27 @@ static int dio_complete(struct dio *dio,
  * Called when a BIO has been processed.  If the count goes to zero then IO is
  * complete and we can signal this to the AIO layer.
  */
-static void finished_one_bio(struct dio *dio)
+static void dio_complete_aio(struct dio *dio)
 {
 	unsigned long flags;
+	int ret;
 
-	spin_lock_irqsave(&dio->bio_lock, flags);
-	if (dio->bio_count == 1) {
-		if (dio->is_async) {
-			int ret;
-
-			/*
-			 * Last reference to the dio is going away.
-			 * Drop spinlock and complete the DIO.
-			 */
-			spin_unlock_irqrestore(&dio->bio_lock, flags);
-
-			ret = dio_complete(dio, dio->iocb->ki_pos, 0);
+	ret = dio_complete(dio, dio->iocb->ki_pos, 0);
 
-			/* Complete AIO later if falling back to buffered i/o */
-			if (dio->result == dio->size ||
-				((dio->rw == READ) && dio->result)) {
-				aio_complete(dio->iocb, ret, 0);
-				kfree(dio);
-				return;
-			} else {
-				/*
-				 * Falling back to buffered
-				 */
-				spin_lock_irqsave(&dio->bio_lock, flags);
-				dio->bio_count--;
-				if (dio->waiter)
-					wake_up_process(dio->waiter);
-				spin_unlock_irqrestore(&dio->bio_lock, flags);
-				return;
-			}
-		}
+	/* Complete AIO later if falling back to buffered i/o */
+	if (dio->result == dio->size ||
+		((dio->rw == READ) && dio->result)) {
+		aio_complete(dio->iocb, ret, 0);
+		kfree(dio);
+	} else {
+		/*
+		 * Falling back to buffered
+		 */
+		spin_lock_irqsave(&dio->bio_lock, flags);
+		if (dio->waiter)
+			wake_up_process(dio->waiter);
+		spin_unlock_irqrestore(&dio->bio_lock, flags);
 	}
-	dio->bio_count--;
-	spin_unlock_irqrestore(&dio->bio_lock, flags);
 }
 
 static int dio_bio_complete(struct dio *dio, struct bio *bio);
@@ -308,6 +290,10 @@ static int dio_bio_end_aio(struct bio *b
 
 	/* cleanup the bio */
 	dio_bio_complete(dio, bio);
+
+	if (atomic_dec_and_test(&dio->refcount))
+		dio_complete_aio(dio);
+
 	return 0;
 }
 
@@ -329,8 +315,7 @@ static int dio_bio_end_io(struct bio *bi
 	spin_lock_irqsave(&dio->bio_lock, flags);
 	bio->bi_private = dio->bio_list;
 	dio->bio_list = bio;
-	dio->bios_in_flight--;
-	if (dio->waiter && dio->bios_in_flight == 0)
+	if ((atomic_sub_return(1, &dio->refcount) == 1) && dio->waiter)
 		wake_up_process(dio->waiter);
 	spin_unlock_irqrestore(&dio->bio_lock, flags);
 	return 0;
@@ -361,17 +346,15 @@ dio_bio_alloc(struct dio *dio, struct bl
  * In the AIO read case we speculatively dirty the pages before starting IO.
  * During IO completion, any of these pages which happen to have been written
  * back will be redirtied by bio_check_pages_dirty().
+ *
+ * bios hold a dio reference between submit_bio and ->end_io.
  */
 static void dio_bio_submit(struct dio *dio)
 {
 	struct bio *bio = dio->bio;
-	unsigned long flags;
 
 	bio->bi_private = dio;
-	spin_lock_irqsave(&dio->bio_lock, flags);
-	dio->bio_count++;
-	dio->bios_in_flight++;
-	spin_unlock_irqrestore(&dio->bio_lock, flags);
+	atomic_inc(&dio->refcount);
 	if (dio->is_async && dio->rw == READ)
 		bio_set_pages_dirty(bio);
 	submit_bio(dio->rw, bio);
@@ -389,18 +372,28 @@ static void dio_cleanup(struct dio *dio)
 		page_cache_release(dio_get_page(dio));
 }
 
+static int wait_for_more_bios(struct dio *dio)
+{
+	assert_spin_locked(&dio->bio_lock);
+
+	return (atomic_read(&dio->refcount) > 1) && (dio->bio_list == NULL);
+}
+
 /*
- * Wait for the next BIO to complete.  Remove it and return it.
+ * Wait for the next BIO to complete.  Remove it and return it.  NULL is
+ * returned once all BIOs have been completed.  This must only be called once
+ * all bios have been issued so that dio->refcount can only decrease.  This
+ * requires that that the caller hold a reference on the dio.
  */
 static struct bio *dio_await_one(struct dio *dio)
 {
 	unsigned long flags;
-	struct bio *bio;
+	struct bio *bio = NULL;
 
 	spin_lock_irqsave(&dio->bio_lock, flags);
-	while (dio->bio_list == NULL) {
+	while (wait_for_more_bios(dio)) {
 		set_current_state(TASK_UNINTERRUPTIBLE);
-		if (dio->bio_list == NULL) {
+		if (wait_for_more_bios(dio)) {
 			dio->waiter = current;
 			spin_unlock_irqrestore(&dio->bio_lock, flags);
 			io_schedule();
@@ -409,8 +402,10 @@ static struct bio *dio_await_one(struct 
 		}
 		set_current_state(TASK_RUNNING);
 	}
-	bio = dio->bio_list;
-	dio->bio_list = bio->bi_private;
+	if (dio->bio_list) {
+		bio = dio->bio_list;
+		dio->bio_list = bio->bi_private;
+	}
 	spin_unlock_irqrestore(&dio->bio_lock, flags);
 	return bio;
 }
@@ -439,25 +434,24 @@ static int dio_bio_complete(struct dio *
 		}
 		bio_put(bio);
 	}
-	finished_one_bio(dio);
 	return uptodate ? 0 : -EIO;
 }
 
 /*
- * Wait on and process all in-flight BIOs.
+ * Wait on and process all in-flight BIOs.  This must only be called once
+ * all bios have been issued so that the refcount can only decrease.
+ * This just waits for all bios to make it through dio_bio_complete.  IO
+ * errors are propogated through dio->io_error and should be propogated via
+ * dio_complete().
  */
 static void dio_await_completion(struct dio *dio)
 {
-	/*
-	 * The bio_lock is not held for the read of bio_count.
-	 * This is ok since it is the dio_bio_complete() that changes
-	 * bio_count.
-	 */
-	while (dio->bio_count) {
-		struct bio *bio = dio_await_one(dio);
-		/* io errors are propogated through dio->io_error */
-		dio_bio_complete(dio, bio);
-	}
+	struct bio *bio;
+	do {
+		bio = dio_await_one(dio);
+		if (bio)
+			dio_bio_complete(dio, bio);
+	} while (bio);
 }
 
 /*
@@ -987,16 +981,7 @@ direct_io_worker(int rw, struct kiocb *i
 	dio->iocb = iocb;
 	dio->i_size = i_size_read(inode);
 
-	/*
-	 * BIO completion state.
-	 *
-	 * ->bio_count starts out at one, and we decrement it to zero after all
-	 * BIOs are submitted.  This to avoid the situation where a really fast
-	 * (or synchronous) device could take the count to zero while we're
-	 * still submitting BIOs.
-	 */
-	dio->bio_count = 1;
-	dio->bios_in_flight = 0;
+	atomic_set(&dio->refcount, 1);
 	spin_lock_init(&dio->bio_lock);
 	dio->bio_list = NULL;
 	dio->waiter = NULL;
@@ -1103,7 +1088,11 @@ direct_io_worker(int rw, struct kiocb *i
 		}
 		if (ret == 0)
 			ret = dio->result;
-		finished_one_bio(dio);		/* This can free the dio */
+
+		/* this can free the dio */
+		if (atomic_dec_and_test(&dio->refcount))
+			dio_complete_aio(dio);
+
 		if (should_wait) {
 			unsigned long flags;
 			/*
@@ -1114,7 +1103,7 @@ direct_io_worker(int rw, struct kiocb *i
 
 			spin_lock_irqsave(&dio->bio_lock, flags);
 			set_current_state(TASK_UNINTERRUPTIBLE);
-			while (dio->bio_count) {
+			while (atomic_read(&dio->refcount)) {
 				spin_unlock_irqrestore(&dio->bio_lock, flags);
 				io_schedule();
 				spin_lock_irqsave(&dio->bio_lock, flags);
@@ -1125,7 +1114,6 @@ direct_io_worker(int rw, struct kiocb *i
 			kfree(dio);
 		}
 	} else {
-		finished_one_bio(dio);
 		dio_await_completion(dio);
 
 		ret = dio_complete(dio, offset, ret);
@@ -1138,7 +1126,11 @@ direct_io_worker(int rw, struct kiocb *i
 			 * i/o, we have to mark the the aio complete.
 			 */
 			aio_complete(iocb, ret, 0);
-		kfree(dio);
+
+		if (atomic_dec_and_test(&dio->refcount))
+			kfree(dio);
+		else
+			BUG();
 	}
 	return ret;
 }

[RFC 4/5] dio: remove duplicate bio wait code

[Zach Brown]

dio: remove duplicate bio wait code

Now that we have a single refcount and waiting path we can reuse it in the
async 'should_wait' path.  It continues to rely on the fragile link between the
conditional in dio_complete_aio() which decides to complete the AIO and the
conditional in direct_io_worker() which decides to wait and free.

By waiting before dropping the reference we stop dio_bio_end_aio() from calling
dio_complete_aio() which used to wake up the waiter after seeing the reference
count drop to 0.  We hoist this wake up into dio_bio_end_aio() which now
notices when it's left a single remaining reference that is held by the waiter.

Signed-off-by: Zach Brown <zach.brown@oracle.com>
---

 fs/direct-io.c |   41 ++++++++++++-----------------------------
 1 file changed, 12 insertions(+), 29 deletions(-)

Index: 2.6.18-rc6-dio-cleanup/fs/direct-io.c
===================================================================
--- 2.6.18-rc6-dio-cleanup.orig/fs/direct-io.c
+++ 2.6.18-rc6-dio-cleanup/fs/direct-io.c
@@ -256,7 +256,6 @@ static int dio_complete(struct dio *dio,
  */
 static void dio_complete_aio(struct dio *dio)
 {
-	unsigned long flags;
 	int ret;
 
 	ret = dio_complete(dio, dio->iocb->ki_pos, 0);
@@ -266,14 +265,6 @@ static void dio_complete_aio(struct dio 
 		((dio->rw == READ) && dio->result)) {
 		aio_complete(dio->iocb, ret, 0);
 		kfree(dio);
-	} else {
-		/*
-		 * Falling back to buffered
-		 */
-		spin_lock_irqsave(&dio->bio_lock, flags);
-		if (dio->waiter)
-			wake_up_process(dio->waiter);
-		spin_unlock_irqrestore(&dio->bio_lock, flags);
 	}
 }
 
@@ -284,6 +275,8 @@ static int dio_bio_complete(struct dio *
 static int dio_bio_end_aio(struct bio *bio, unsigned int bytes_done, int error)
 {
 	struct dio *dio = bio->bi_private;
+	int waiter_holds_ref = 0;
+	int remaining;
 
 	if (bio->bi_size)
 		return 1;
@@ -291,7 +284,12 @@ static int dio_bio_end_aio(struct bio *b
 	/* cleanup the bio */
 	dio_bio_complete(dio, bio);
 
-	if (atomic_dec_and_test(&dio->refcount))
+	waiter_holds_ref = !!dio->waiter;
+	remaining = atomic_sub_return(1, (&dio->refcount));
+	if (remaining == 1 && waiter_holds_ref)
+		wake_up_process(dio->waiter);
+
+	if (remaining == 0)
 		dio_complete_aio(dio);
 
 	return 0;
@@ -1089,30 +1087,15 @@ direct_io_worker(int rw, struct kiocb *i
 		if (ret == 0)
 			ret = dio->result;
 
+		if (should_wait)
+			dio_await_completion(dio);
+
 		/* this can free the dio */
 		if (atomic_dec_and_test(&dio->refcount))
 			dio_complete_aio(dio);
 
-		if (should_wait) {
-			unsigned long flags;
-			/*
-			 * Wait for already issued I/O to drain out and
-			 * release its references to user-space pages
-			 * before returning to fallback on buffered I/O
-			 */
-
-			spin_lock_irqsave(&dio->bio_lock, flags);
-			set_current_state(TASK_UNINTERRUPTIBLE);
-			while (atomic_read(&dio->refcount)) {
-				spin_unlock_irqrestore(&dio->bio_lock, flags);
-				io_schedule();
-				spin_lock_irqsave(&dio->bio_lock, flags);
-				set_current_state(TASK_UNINTERRUPTIBLE);
-			}
-			spin_unlock_irqrestore(&dio->bio_lock, flags);
-			set_current_state(TASK_RUNNING);
+		if (should_wait)
 			kfree(dio);
-		}
 	} else {
 		dio_await_completion(dio);
 

[RFC 5/5] dio: only call aio_complete() after returning -EIOCBQUEUED

[Zach Brown]

dio: only call aio_complete() after returning -EIOCBQUEUED

The only time it is safe to call aio_complete() is when the ->ki_retry function
returns -EIOCBQUEUED to the AIO core.  direct_io_worker() has historically done
this by relying on its caller to translate positive return codes into
-EIOCBQUEUED for the aio case.  It did this by trying to keep conditionals in
sync.  direct_io_worker() knew when finished_one_bio() was going to call
aio_complete().  It would reverse the test and wait and free the dio in the
cases it thought that finished_one_bio() wasn't going to.

Not surprisingly, it ended up getting it wrong.  'ret' could be a negative
errno from the submission path but it failed to communicate this to
finished_one_bio().  direct_io_worker() would return < 0, it's callers wouldn't
raise -EIOCBQUEUED, and aio_complete() would be called.  In the future
finished_one_bio()'s tests wouldn't reflect this and aio_complete() would be
called for a second time which can manifest as an oops.

The previous cleanups have whittled the sync and async completion paths down to
the point where we can collapse them and clearly reassert the invariant that we
must only call aio_complete() after returning -EIOCBQUEUED.  direct_io_worker()
will only return -EIOCBQUEUED when it is not the last to drop the dio refcount
and the aio bio completion path will only call aio_complete() when it is the
last to drop the dio refcount.  direct_io_worker() can ensure that it is the
last to drop the reference count by waiting for bios to drain.  It does this
for sync ops, of course, and for partial dio writes that must fall back to
buffered and for aio ops that saw errors during submission.

This means that operations that end up waiting, even if they were issued as aio
ops, will not call aio_complete() from dio.  Instead we return the return code
of the operation and let the aio core call aio_complete().  This is purposely
done to fix a bug where AIO DIO file extensions would call aio_complete()
before their callers have a chance to update i_size.

Now that direct_io_worker() is explicitly returning -EIOCBQUEUED its callers no
longer have to translate for it.

Signed-off-by: Zach Brown <zach.brown@oracle.com>
---

 fs/direct-io.c |   92 ++++++++++++++++++-----------------------------
 mm/filemap.c   |    4 --
 2 files changed, 36 insertions(+), 60 deletions(-)

Index: 2.6.18-rc6-dio-cleanup/fs/direct-io.c
===================================================================
--- 2.6.18-rc6-dio-cleanup.orig/fs/direct-io.c
+++ 2.6.18-rc6-dio-cleanup/fs/direct-io.c
@@ -225,6 +225,15 @@ static int dio_complete(struct dio *dio,
 {
 	ssize_t transferred = 0;
 
+	/*
+	 * AIO submission can race with bio completion to get here while
+	 * expecting to have the last io completed by bio completion.
+	 * In that case -EIOCBQUEUED is in fact not an error we want
+	 * to preserve through this call.
+	 */
+	if (ret == -EIOCBQUEUED)
+		ret = 0;
+
 	if (dio->result) {
 		transferred = dio->result;
 
@@ -250,24 +259,6 @@ static int dio_complete(struct dio *dio,
 	return ret;
 }
 
-/*
- * Called when a BIO has been processed.  If the count goes to zero then IO is
- * complete and we can signal this to the AIO layer.
- */
-static void dio_complete_aio(struct dio *dio)
-{
-	int ret;
-
-	ret = dio_complete(dio, dio->iocb->ki_pos, 0);
-
-	/* Complete AIO later if falling back to buffered i/o */
-	if (dio->result == dio->size ||
-		((dio->rw == READ) && dio->result)) {
-		aio_complete(dio->iocb, ret, 0);
-		kfree(dio);
-	}
-}
-
 static int dio_bio_complete(struct dio *dio, struct bio *bio);
 /*
  * Asynchronous IO callback. 
@@ -289,8 +280,11 @@ static int dio_bio_end_aio(struct bio *b
 	if (remaining == 1 && waiter_holds_ref)
 		wake_up_process(dio->waiter);
 
-	if (remaining == 0)
-		dio_complete_aio(dio);
+	if (remaining == 0) {
+		int ret = dio_complete(dio, dio->iocb->ki_pos, 0);
+		aio_complete(dio->iocb, ret, 0);
+		kfree(dio);
+	}
 
 	return 0;
 }
@@ -1074,47 +1068,33 @@ direct_io_worker(int rw, struct kiocb *i
 		mutex_unlock(&dio->inode->i_mutex);
 
 	/*
-	 * OK, all BIOs are submitted, so we can decrement bio_count to truly
-	 * reflect the number of to-be-processed BIOs.
-	 */
-	if (dio->is_async) {
-		int should_wait = 0;
-
-		if (dio->result < dio->size && (rw & WRITE)) {
-			dio->waiter = current;
-			should_wait = 1;
-		}
-		if (ret == 0)
-			ret = dio->result;
-
-		if (should_wait)
-			dio_await_completion(dio);
-
-		/* this can free the dio */
-		if (atomic_dec_and_test(&dio->refcount))
-			dio_complete_aio(dio);
+	 * The only time we want to leave bios in flight is when a successful
+	 * partial aio read or full aio write have been setup.  In that case
+	 * bio completion will call aio_complete.  The only time it's safe to
+	 * call aio_complete is when we return -EIOCBQUEUED, so we key on that.
+	 * This had *better* be the only place that raises -EIOCBQUEUED.
+	 */
+	BUG_ON(ret == -EIOCBQUEUED);
+	if (dio->is_async && ret == 0 && dio->result &&
+	    ((rw & READ) || (dio->result == dio->size)))
+		ret = -EIOCBQUEUED;
 
-		if (should_wait)
-			kfree(dio);
-	} else {
+	if (ret != -EIOCBQUEUED)
 		dio_await_completion(dio);
 
+	/*
+	 * Sync will always be dropping the final ref and completing the
+	 * operation.  AIO can if it was a broken operation described above
+	 * or in fact if all the bios race to complete before we get here.
+	 * In that case dio_complete() translates the EIOCBQUEUED into
+	 * the proper return code that the caller will hand to aio_complete().
+	 */
+	if (atomic_dec_and_test(&dio->refcount)) {
 		ret = dio_complete(dio, offset, ret);
+		kfree(dio);
+	} else
+		BUG_ON(ret != -EIOCBQUEUED);
 
-		/* We could have also come here on an AIO file extend */
-		if (!is_sync_kiocb(iocb) && (rw & WRITE) &&
-		    ret >= 0 && dio->result == dio->size)
-			/*
-			 * For AIO writes where we have completed the
-			 * i/o, we have to mark the the aio complete.
-			 */
-			aio_complete(iocb, ret, 0);
-
-		if (atomic_dec_and_test(&dio->refcount))
-			kfree(dio);
-		else
-			BUG();
-	}
 	return ret;
 }
 
Index: 2.6.18-rc6-dio-cleanup/mm/filemap.c
===================================================================
--- 2.6.18-rc6-dio-cleanup.orig/mm/filemap.c
+++ 2.6.18-rc6-dio-cleanup/mm/filemap.c
@@ -1175,8 +1175,6 @@ __generic_file_aio_read(struct kiocb *io
 		if (pos < size) {
 			retval = generic_file_direct_IO(READ, iocb,
 						iov, pos, nr_segs);
-			if (retval > 0 && !is_sync_kiocb(iocb))
-				retval = -EIOCBQUEUED;
 			if (retval > 0)
 				*ppos = pos + retval;
 		}
@@ -2053,8 +2051,6 @@ generic_file_direct_write(struct kiocb *
 		if (err < 0)
 			written = err;
 	}
-	if (written == count && !is_sync_kiocb(iocb))
-		written = -EIOCBQUEUED;
 	return written;
 }
 EXPORT_SYMBOL(generic_file_direct_write);

Re: bogofilter ate 3/5

[Zach Brown]

There was a 3/5 but the bogofilter decided it was spam.  It made it into
the linux-aio archive:

  http://marc.theaimsgroup.com/?l=linux-aio&m=115750084710650&w=2

What should I have done to avoid the spam regexes and what should I do
now that I have a patch that makes them angry?

- z

Re: bogofilter ate 3/5

[Willy Tarreau]

On Tue, Sep 05, 2006 at 09:35:37PM -0700, Zach Brown wrote:
> 
> There was a 3/5 but the bogofilter decided it was spam.  It made it into
> the linux-aio archive:
> 
>   http://marc.theaimsgroup.com/?l=linux-aio&m=115750084710650&w=2
> 
> What should I have done to avoid the spam regexes and what should I do
> now that I have a patch that makes them angry?

I don't know. IMHO, bogofilter should only add a header so that people
who want to filter can and those who don't want to will get all the
messages. Spam has never been a real problem for me on LKML, but loss
of messages and patches will certainly be. I would rather have the choice
to not filter anything :-/

Willy

Re: bogofilter ate 3/5

[Mike Galbraith]

On Tue, 2006-09-05 at 21:35 -0700, Zach Brown wrote:
> What should I have done to avoid the spam regexes and what should I do
> now that I have a patch that makes them angry?

Use language similar to the naughty stuff that is getting through?

Seriously though, a quote from Matti's lkml announcement:

IF we take it into use, it will start rejecting messages
at SMTP input phase, so if it rejects legitimate message,
you should get a bounce from your email provider's system.
(Or from zeus.kernel.org, which is vger's backup MX.)

In such case, send the bounce with some explanations to 
<postmaster@vger.kernel.org> -- emails to that address
are explicitely excluded from all filtering!

Re: bogofilter ate 3/5

[Matthias Andree]

On Tue, 05 Sep 2006, Zach Brown wrote:

> There was a 3/5 but the bogofilter decided it was spam.

Certainly not. The training data you fed into bogofilter data made that
message look like spam. -- Yes, that's a difference.

-- 
Matthias Andree

Re: [RFC 0/5] dio: clean up completion phase of direct_io_worker()

[Suparna Bhattacharya]

On Tue, Sep 05, 2006 at 04:57:32PM -0700, Zach Brown wrote:
> There have been a lot of bugs recently due to the way direct_io_worker() tries
> to decide how to finish direct IO operations.  In the worst examples it has
> failed to call aio_complete() at all (hang) or called it too many times (oops).
> 
> This set of patches cleans up the completion phase with the goal of removing
> the complexity that lead to these bugs.  We end up with one path that
> calculates the result of the operation after all off the bios have completed.
> We decide when to generate a result of the operation using that path based on
> the final release of a refcount on the dio structure.

Very nice piece of work ! Thanks for taking this up :)

I have looked through all the patches and the completion path unification
logic looks sound to me (btw, the explanations and comments are very good too).
Not the usual bandaids that we often end up with, but a real cleanup that
should go some way in making at least the most errorprone part of the DIO
code more maintainable (I hope/wish we could also do something similar with
simplifying the locking as well).

Of course with this code, we have to await rigorous testing
... and more reviews, but please consider this as my ack for the approach.

Regards
Suparna

> 
> I tried to progress towards the final state in steps that were relatively easy
> to understand.  Each step should compile but I only tested the final result of
> having all the patches applied.
> 
> The patches result in a slight net decrease in code and binary size:
> 
>  2.6.18-rc4-dio-cleanup/fs/direct-io.c |    8
>  2.6.18-rc5-dio-cleanup/fs/direct-io.c |   94 +++++------
>  2.6.18-rc5-dio-cleanup/mm/filemap.c   |    4
>  fs/direct-io.c                        |  273 ++++++++++++++--------------------
>  4 files changed, 159 insertions(+), 220 deletions(-)
> 
>    text    data     bss     dec     hex filename
> 2592385  450996  210296 3253677  31a5ad vmlinux.before
> 2592113  450980  210296 3253389  31a48d vmlinux.after
> 
> The patches pass light testing with aio-stress, the direct IO tests I could
> manage to get running in LTP, and some home-brew functional tests.  It's still
> making its way through stress testing.  It should not be merged until we hear
> from that more rigorous testing, I don't think.
> 
> I hoped to get some feedback (and maybe volunteers for testing!) by sending the
> patches out before waiting for the stress tests.
> 
> - z
> 
> --
> To unsubscribe, send a message with 'unsubscribe linux-aio' in
> the body to majordomo@kvack.org.  For more info on Linux AIO,
> see: http://www.kvack.org/aio/
> Don't email: <a href=mailto:"aart@kvack.org">aart@kvack.org</a>

-- 
Suparna Bhattacharya (suparna@in.ibm.com)
Linux Technology Center
IBM Software Lab, India

Re: [RFC 0/5] dio: clean up completion phase of direct_io_worker()

[Zach Brown]

> code more maintainable (I hope/wish we could also do something similar with
> simplifying the locking as well).

I agree, and that is definitely on my medium-term todo list.

> Of course with this code, we have to await rigorous testing
> ... and more reviews, but please consider this as my ack for the approach.

Yeah, absolutely.  Is there a chance that IBM can throw some testing
cycles at it?  I have it queued up for some moderately sized DB runs
over here (FC arrays, cable pulling, that kind of thing.)

- z

Re: [RFC 0/5] dio: clean up completion phase of direct_io_worker()

[Jeff Moyer]

==> Regarding [RFC 0/5] dio: clean up completion phase of direct_io_worker(); Zach Brown <zach.brown@oracle.com> adds:

zach.brown> There have been a lot of bugs recently due to the way
zach.brown> direct_io_worker() tries to decide how to finish direct IO
zach.brown> operations.  In the worst examples it has failed to call
zach.brown> aio_complete() at all (hang) or called it too many times
zach.brown> (oops).

zach.brown> This set of patches cleans up the completion phase with the
zach.brown> goal of removing the complexity that lead to these bugs.  We
zach.brown> end up with one path that calculates the result of the
zach.brown> operation after all off the bios have completed.  We decide
zach.brown> when to generate a result of the operation using that path
zach.brown> based on the final release of a refcount on the dio structure.

[...]

zach.brown> I hoped to get some feedback (and maybe volunteers for
zach.brown> testing!) by sending the patches out before waiting for the
zach.brown> stress tests.

This all looks good, the code is much easier to follow.  What do you think
about making dio->result an unsigned quantity?  It should never be negative
now that there is an io_error field.

ACK.

Jeff

Re: [RFC 0/5] dio: clean up completion phase of direct_io_worker()

[Zach Brown]

> This all looks good, the code is much easier to follow.  What do you think
> about making dio->result an unsigned quantity?  It should never be negative
> now that there is an io_error field.

Yeah, that has always bugged me too.  I considered renaming it 'issued',
or something, as part of this patchset but thought we could do it later.

While we're on this topic, I'm nervious that we increment it when
do_direct_IO fails.  It might be sound, but that we consider it the
amount of work "transferred" for dio->end_io makes me want to make sure
there aren't confusing corner cases here.

- z

Re: [RFC 0/5] dio: clean up completion phase of direct_io_worker()

[Jeff Moyer]

==> Regarding Re: [RFC 0/5] dio: clean up completion phase of direct_io_worker(); Zach Brown <zach.brown@oracle.com> adds:

>> This all looks good, the code is much easier to follow.  What do you think
>> about making dio->result an unsigned quantity?  It should never be negative
>> now that there is an io_error field.

zach.brown> Yeah, that has always bugged me too.  I considered renaming it
zach.brown> 'issued', or something, as part of this patchset but thought we
zach.brown> could do it later.

I figured since you were doing some house-keeping, we might as well clean
up as much as possible.  It's up to you, though.  ;)

zach.brown> While we're on this topic, I'm nervious that we increment it
zach.brown> when do_direct_IO fails.  It might be sound, but that we
zach.brown> consider it the amount of work "transferred" for dio->end_io
zach.brown> makes me want to make sure there aren't confusing corner cases
zach.brown> here.

It does look non-obvious when reading the code.  However, I'm pretty sure
it's right.  dio->block_in_file is only updated if there is no error
returned from submit_page_section.  As such, it really does reflect how
much work was done before the error, right?  It does seem odd that we do
this math in two separate places, though.

-Jeff

Re: bogofilter ate 3/5

[Rick Ellis]

On Wed, 6 Sep 2006 07:00:44 +0200 Willy Tarreau wrote:

> spam has never been a real problem for me on LKML

I spend a lot of time deleting spam from the mailing lists I
archive. The kernel list gets a lot of spam--it's just not
as obvious as it is on some lists because of the large amount
of legit traffic. Some of the slower lists get nothing but
spam for weeks at a time.

While bogofilter may not be the answer, doing something may
be quite desirable.

--
http://www.spinics.net/lists/kernel/

Re: bogofilter ate 3/5

[Willy Tarreau]

On Tue, Sep 05, 2006 at 10:37:03PM -0700, Rick Ellis wrote:
> On Wed, 6 Sep 2006 07:00:44 +0200 Willy Tarreau wrote:
> 
> > spam has never been a real problem for me on LKML
> 
> I spend a lot of time deleting spam from the mailing lists I
> archive. The kernel list gets a lot of spam--it's just not
> as obvious as it is on some lists because of the large amount
> of legit traffic. Some of the slower lists get nothing but
> spam for weeks at a time.

agreed, but I still think that losing some legitimate emails
is worse than getting 5% spam.

> While bogofilter may not be the answer, doing something may
> be quite desirable.

OK, but doing something could simply consist in adding a header
that anyone is free to filter on or not.

Regards,
Willy

Re: bogofilter ate 3/5

[Willy Tarreau]

On Wed, Sep 06, 2006 at 08:00:20PM +0200, Willy Tarreau wrote:
> On Tue, Sep 05, 2006 at 10:37:03PM -0700, Rick Ellis wrote:
> > On Wed, 6 Sep 2006 07:00:44 +0200 Willy Tarreau wrote:
> > 
> > > spam has never been a real problem for me on LKML
> > 
> > I spend a lot of time deleting spam from the mailing lists I
> > archive. The kernel list gets a lot of spam--it's just not
> > as obvious as it is on some lists because of the large amount
> > of legit traffic. Some of the slower lists get nothing but
> > spam for weeks at a time.
> 
> agreed, but I still think that losing some legitimate emails
> is worse than getting 5% spam.
> 
> > While bogofilter may not be the answer, doing something may
> > be quite desirable.
> 
> OK, but doing something could simply consist in adding a header
> that anyone is free to filter on or not.

BTW, I forgot to say that I fear that maintaining the bogofilter
in the long term will be supplementary work for Matti, with few
chances of 100% success with happy readers.

Regards,
Willy

Re: bogofilter ate 3/5

[ellis]

> OK, but doing something could simply consist in adding a header
> that anyone is free to filter on or not.

The problem with that is the post gets no indication that his
mail has been filtered. The way it works now is the rejection
happens at SMTP time and that causes the poster to see the
problem. If people filtered on a header, you'd never know why you
weren't getting a response.

--
http://www.spinics.net/lists/kernel/

Re: bogofilter ate 3/5

[Chase Venters]

On Wed, 6 Sep 2006, ellis@spinics.net wrote:

>> OK, but doing something could simply consist in adding a header
>> that anyone is free to filter on or not.
>
> The problem with that is the post gets no indication that his
> mail has been filtered. The way it works now is the rejection
> happens at SMTP time and that causes the poster to see the
> problem. If people filtered on a header, you'd never know why you
> weren't getting a response.
>

How about this:

1. Incoming mail from subscribers is accepted
2. Incoming mail to honeypot addresses is trained as SPAM
3. Incoming mail from non-subscribers is marked with X-Bogofilter:
4. A handy Perl script subscribes to lkml, and for any message it gets 
with an X-Bogofilter: SPAM header, it sends a notification (rate-limited) 
to the message sender explaining that his message will be filtered as SPAM 
by some recipients, and inviting him to contact postmaster to resolve the 
issue, and additionally letting him know that notification is rate-limited 
and there is a website he can check to see the SUBJECTs of all messages 
filtered as SPAM on lkml (say for the last week or two) if he wants to try 
and correct the problem himself.

Thanks,
Chase

Re: bogofilter ate 3/5

[Krzysztof Halasa]

Chase Venters <chase.venters@clientec.com> writes:

> 1. Incoming mail from subscribers is accepted

How do you know if the sender is really a subscriber?

> 4. A handy Perl script subscribes to lkml, and for any message it gets
> with an X-Bogofilter: SPAM header, it sends a notification
> (rate-limited) to the message sender

How do you know who the sender really is? IMHO bouncing anything
(especially spam) after SMTP OK is worse than the spam itself.
-- 
Krzysztof Halasa

Re: bogofilter ate 3/5

[Chase Venters]

On Wed, 6 Sep 2006, Krzysztof Halasa wrote:

> Chase Venters <chase.venters@clientec.com> writes:
>
>> 1. Incoming mail from subscribers is accepted
>
> How do you know if the sender is really a subscriber?

You can check the From: or envelope sender against the subscriber 
database. Forgery isn't a concern because we're not trying to stop 
forgery with this method. Subscribers subscribing one address and sending 
from another is also not a problem since a lookup failure just means you 
get to ride through the bogofilter. Note as well that #4 is a separate 
program; this lookup is likely done by the mailing list software.

#1 should significantly reduce the load on the bogofilter (not sure if 
that matters though).

>> 4. A handy Perl script subscribes to lkml, and for any message it gets
>> with an X-Bogofilter: SPAM header, it sends a notification
>> (rate-limited) to the message sender
>
> How do you know who the sender really is? IMHO bouncing anything
> (especially spam) after SMTP OK is worse than the spam itself.
>

The perl script behaves as an optional autoresponder. Autoresponders would 
respond to spam as well (well, unless you put a spam filter in front of 
them, but I assume that many don't).

Also note that a number of people (myself included, at work anyway) have 
perl scripts that respond to all incoming mail and require a reply cookie from original 
envelope senders. We do it because it almost entirely prevents spam from 
arriving in our inboxes (I say almost because there is the occasional 
spammer that doesn't forge their sender address and has some kind of 
autoresponder behind it). I had to do this for my work account to stop the
hundreds of messages I was getting each day after a co-worker "pranked me" 
by signing me up for all that crap.

Thanks,
Chase

Re: bogofilter ate 3/5

[Krzysztof Halasa]

Chase Venters <chase.venters@clientec.com> writes:

> You can check the From: or envelope sender against the subscriber
> database. Forgery isn't a concern because we're not trying to stop
> forgery with this method.

That's the first problem.

> The perl script behaves as an optional autoresponder. Autoresponders
> would respond to spam as well (well, unless you put a spam filter in
> front of them, but I assume that many don't).

Yep. Sending their "responses" to innocent people, instead of spam
senders. That's what many "antivirus" do.

> Also note that a number of people (myself included, at work anyway)
> have perl scripts that respond to all incoming mail and require a
> reply cookie from original envelope senders. We do it because it
> almost entirely prevents spam from arriving in our inboxes

Sure. Don't you think is also prevents a lot of legitimate mail?
Hope that all addresses you send mail to are automatically added
to a white list? (I'm especially annoyed with people asking me for
something, and then my answer bounces with "click somewhere"
response).
-- 
Krzysztof Halasa

Re: bogofilter ate 3/5

[Chase Venters]

On Thu, 7 Sep 2006, Krzysztof Halasa wrote:

> Chase Venters <chase.venters@clientec.com> writes:
>
>> You can check the From: or envelope sender against the subscriber
>> database. Forgery isn't a concern because we're not trying to stop
>> forgery with this method.
>
> That's the first problem.
>

The problem with trying to stop forgery is that there is not yet a 
foolproof or reasonably foolproof method of doing so. All available 
options require cooperation from other MTAs, MUAs, users or 
administrators; hence, they're only good at increasing 'HAM' score.

It is also at least partially unapplicable to LKML since this list 
deliberately allows anyone to post.

>> The perl script behaves as an optional autoresponder. Autoresponders
>> would respond to spam as well (well, unless you put a spam filter in
>> front of them, but I assume that many don't).
>
> Yep. Sending their "responses" to innocent people, instead of spam
> senders. That's what many "antivirus" do.
>
>> Also note that a number of people (myself included, at work anyway)
>> have perl scripts that respond to all incoming mail and require a
>> reply cookie from original envelope senders. We do it because it
>> almost entirely prevents spam from arriving in our inboxes
>
> Sure. Don't you think is also prevents a lot of legitimate mail?
> Hope that all addresses you send mail to are automatically added
> to a white list? (I'm especially annoyed with people asking me for
> something, and then my answer bounces with "click somewhere"
> response).
>

Yeah, I whitelist on that address as well. And if someone does respond to 
the challenge (all it takes is reply/send, as I've used a signed VERP as 
the Reply-To), they get whitelisted. If the challenge bounces, it bounces 
to the envelope sender (a different signed VERP) which increases their 
blacklist score by 1. Get a blacklist score of 3 and I'll silently ignore 
your mail for 6 months.

Thanks,
Chase

Re: bogofilter ate 3/5

[Krzysztof Halasa]

Chase Venters <chase.venters@clientec.com> writes:

> The problem with trying to stop forgery is that there is not yet a
> foolproof or reasonably foolproof method of doing so.

The first important point here is that vger rejects mail in SMTP
DATA phase, thus making the forgery irrelevant WRT such bounces.
Second, being on the list isn't enough for the message to go
through, it has to pass the filters as everyone else.

Third, while I think "normal" autoresponders (vacation etc.)
are perfectly reasonable (not in mailing list context of course),
ones which by design respond mostly to forged addresses (do you
think antivirus and antispam qualify?) are aimed at the wrong,
innocent person.
-- 
Krzysztof Halasa

Re: bogofilter ate 3/5

[Chase Venters]

On Fri, 8 Sep 2006, Krzysztof Halasa wrote:

> Chase Venters <chase.venters@clientec.com> writes:
>
>> The problem with trying to stop forgery is that there is not yet a
>> foolproof or reasonably foolproof method of doing so.
>
> The first important point here is that vger rejects mail in SMTP
> DATA phase, thus making the forgery irrelevant WRT such bounces.
> Second, being on the list isn't enough for the message to go
> through, it has to pass the filters as everyone else.
>

Fair enough. The discussion drifted with the introduction of the SpamCop 
idea that bounce messages are evil. As you have shown, it is possible in 
many instances to refuse mail at the door; however, it's pretty unfeasible 
in many environments...

>
> Third, while I think "normal" autoresponders (vacation etc.)
> are perfectly reasonable (not in mailing list context of course),
> ones which by design respond mostly to forged addresses (do you
> think antivirus and antispam qualify?) are aimed at the wrong,
> innocent person.
>

Well, this is an interesting point applicable to my specific suggestion. 
You are right that such a script would generate a lot of misdirected mail. 
In my experience in dealing with this issue, it's usually e-mail viruses 
that spoof legitimate addresses: most of the spam I see comes from junk 
addresses at junk domains. So it's not like you're bugging a *human*.

In any case, this specific idea was just a brainstorm for how we could 
use X-Bogofilter: headers to allow LKML subscribers to make their own 
filtering decision about the messages. Another idea is to divert junk mail 
to a second list which is for people that want to be real sure they don't 
miss patches.

A third choice is obviously to leave the problem alone and let bogofilter 
eat our patches ;)

Whichever route is taken, though, it looks like we might have to switch 
away from Majordomo soon, because SpamCop and Stuart MacDonald are going 
to bring an end to programs that automatically respond to mail.

Thanks,
Chase

Re: bogofilter ate 3/5

[Krzysztof Halasa]

Chase Venters <chase.venters@clientec.com> writes:

> A third choice is obviously to leave the problem alone and let
> bogofilter eat our patches ;)

Actually I think Matti will just fix the filter and all such
problems will go away.
-- 
Krzysztof Halasa

Re: bogofilter ate 3/5

[Matti Aarnio]

On Fri, Sep 08, 2006 at 12:58:02AM +0200, Krzysztof Halasa wrote:
> To:	Chase Venters <chase.venters@clientec.com>
> Cc:	ellis@spinics.net, w@1wt.eu (Willy Tarreau),
> 	linux-kernel@vger.kernel.org
> Subject: Re: bogofilter ate 3/5
> From:	Krzysztof Halasa <khc@pm.waw.pl>
> Date:	Fri, 08 Sep 2006 00:58:02 +0200
> 
> Chase Venters <chase.venters@clientec.com> writes:
> 
> > A third choice is obviously to leave the problem alone and let
> > bogofilter eat our patches ;)
> 
> Actually I think Matti will just fix the filter and all such
> problems will go away.

Yes.  Ever so slowly, but eventually.

I have this Feynemanish approach on problem solving.
Gather data, think a lot, try something...

> -- 
> Krzysztof Halasa

  /Matti Aarnio

RE: bogofilter ate 3/5

[Stuart MacDonald]

From: On Behalf Of Chase Venters
> You can check the From: or envelope sender against the subscriber 
> database. Forgery isn't a concern because we're not trying to stop 
> forgery with this method. Subscribers subscribing one address 

Forgery is always a concern...

> The perl script behaves as an optional autoresponder. 
> Autoresponders would 
> respond to spam as well (well, unless you put a spam filter 
> in front of 
> them, but I assume that many don't).

..because autoresponders are always replying to forged addresses:
http://www.spamcop.net/fom-serve/cache/329.html

> Also note that a number of people (myself included, at work 
> anyway) have 
> perl scripts that respond to all incoming mail and require a 
> reply cookie from original 
> envelope senders. We do it because it almost entirely 
> prevents spam from 
> arriving in our inboxes (I say almost because there is the occasional 

Autoresponder by another name, see above URL.

..Stu

RE: bogofilter ate 3/5

[Chase Venters]

On Thu, 7 Sep 2006, Stuart MacDonald wrote:

> From: On Behalf Of Chase Venters
>> You can check the From: or envelope sender against the subscriber
>> database. Forgery isn't a concern because we're not trying to stop
>> forgery with this method. Subscribers subscribing one address
>
> Forgery is always a concern...
>
>> The perl script behaves as an optional autoresponder.
>> Autoresponders would
>> respond to spam as well (well, unless you put a spam filter
>> in front of
>> them, but I assume that many don't).
>
> ..because autoresponders are always replying to forged addresses:
> http://www.spamcop.net/fom-serve/cache/329.html
>
>> Also note that a number of people (myself included, at work
>> anyway) have
>> perl scripts that respond to all incoming mail and require a
>> reply cookie from original
>> envelope senders. We do it because it almost entirely
>> prevents spam from
>> arriving in our inboxes (I say almost because there is the occasional
>
> Autoresponder by another name, see above URL.

I should also point out that common and regular mailing list software 
already often behaves as an autoresponder, and it is completely 
reasonable! Suppose that you send a message to a mailing list that is 
subscriber-only. What usually happens? You get mail back saying that your 
message has been queued for moderator review!

Naturally, such a system suffers from the same problems described by 
the Spamcop page you linked. But it is unreasonable to ask list managers 
not to respond to unknown traffic, because sending a message to a list and 
having it silently disappear is unacceptable.

Now, I'm sure there are some people that don't run mailing lists that 
would love to call this behavior 'bad'. But there are also people who 
would like to rewrite the rules for Internet mail (see: SPF and the 
problem with mail forwarders, and their so-called 'solution'). Since 
Internet mail was designed in a vacuum where these modern problems don't 
exist, we're all forced to work around them in unusual ways. I find it 
highly ironic that spam blocker services tell you not to use certain 
techniques (autoresponders, bounce messages) that are not only 
commonplace, but precedented and even mandated by RFC on the grounds that 
they may cause you to be blocked. Then they move on to criticize anti-spam 
techniques that fall in these domains with one of their subpoints saying 
'they can cause you to miss legitimate mail!'

Guess what: so does indiscriminately blocking people whose sites don't bow 
down to your unreasonable demands, especially when their behavior (say, 
sending bounce messages) is described in the official protocol 
documentation.

Taking away auto-responders is like taking away hair gel from airline 
passengers: a gross overreaction that diminishes the quality of service 
for everyone.

> ..Stu
>

Thanks,
Chase

RE: bogofilter ate 3/5

[Stuart MacDonald]

From: Chase Venters [mailto:chase.venters@clientec.com] 
> highly ironic that spam blocker services tell you not to use certain 
> techniques (autoresponders, bounce messages) that are not only 
> commonplace, but precedented and even mandated by RFC on the 
> grounds that 
> they may cause you to be blocked. Then they move on to 
> criticize anti-spam 
> techniques that fall in these domains with one of their 
> subpoints saying 
> 'they can cause you to miss legitimate mail!'
> 
> Guess what: so does indiscriminately blocking people whose 
> sites don't bow 
> down to your unreasonable demands, especially when their 
> behavior (say, 
> sending bounce messages) is described in the official protocol 
> documentation.

I will assume you are referring to SpamCop. Their service does not
behave the way you think it does:
http://forum.spamcop.net/forums/index.php?autocom=custom&page=whatis
Read the paragraph: "SpamCop works exactly like the credit reporting
agencies..."

..Stu

RE: bogofilter ate 3/5

[Chase Venters]

On Thu, 7 Sep 2006, Stuart MacDonald wrote:

> From: Chase Venters [mailto:chase.venters@clientec.com]
>> highly ironic that spam blocker services tell you not to use certain
>> techniques (autoresponders, bounce messages) that are not only
>> commonplace, but precedented and even mandated by RFC on the
>> grounds that
>> they may cause you to be blocked. Then they move on to
>> criticize anti-spam
>> techniques that fall in these domains with one of their
>> subpoints saying
>> 'they can cause you to miss legitimate mail!'
>>
>> Guess what: so does indiscriminately blocking people whose
>> sites don't bow
>> down to your unreasonable demands, especially when their
>> behavior (say,
>> sending bounce messages) is described in the official protocol
>> documentation.
>
> I will assume you are referring to SpamCop. Their service does not
> behave the way you think it does:
> http://forum.spamcop.net/forums/index.php?autocom=custom&page=whatis
> Read the paragraph: "SpamCop works exactly like the credit reporting
> agencies..."
>

What are you implying - that SpamCop doesn't make decisions about who to 
block and who to not block for third parties? Their weasel wording 
comparing themselves to credit reporting is pretty weak and doesn't stand
a chance of obscuring their purpose, methods or results from anyone with half
a clue. But let's pretend that it did for just long enough that I can 
point out that because of the coercive nature of credit agency decisions, 
they are expected to behave reasonably as well. Saying, "but the creditors 
are making the decisions and we're just providing data!" is no excuse for 
credit reporting agencies to, say, give low credit scores to people who 
live in a certain part of town, or practice a certain religion, or happen 
to have skin of a certain color.

I will strongly criticize any service that purports to label senders of 
automatic responses as senders of unsolicited mail. The response sent by 
an autoresponder (be it a deferral, bounce, vacation or mailing list 
manager message) is most definitely solicited. The fact that Internet mail 
is broken enough to allow terribly easy envelope forgery does not change 
this fact. Trying to defer responsibility for a misdirected automatic 
response to the program or party using the program that sent it is like 
trying to blame gun manufacturers for specific instances of murder; it's 
absurd and it misses the point.

Whether or not SpamCop servers actually drop any messages is irrelevant 
when they are providing purportedly reputable data to third parties who 
use it to decide whether or not to drop messages themselves. The sad fact 
is that most postmasters just aren't educated enough about these issues to 
see through the glossy marketing materials RBLs and other services use to 
promote their wares.

And on the specific issue of autoresponders, I think a reasonable 
compromise is to support DomainKeys. That way if a sender is irritated 
that they are receiving automatic responses from messages they didn't 
send, they can personally take action to invalidate the forgery.

But mark my words: Asking hosts to stop sending bounce messages or 
automatic responses is insane and contrary to over a decade of established 
postmaster precedent.

>
> ..Stu
>

Thanks,
Chase

RE: bogofilter ate 3/5

[Stuart MacDonald]

From: Chase Venters [mailto:chase.venters@clientec.com] 
> What are you implying - that SpamCop doesn't make decisions 
> about who to 
> block and who to not block for third parties? Their weasel wording 

It's not an implication, it's a fact.

> I will strongly criticize any service that purports to label 
> senders of 
> automatic responses as senders of unsolicited mail. The 

What would you call it then when I receive a bounce/etc that is in
reponse to a message someone else sent? I certainly never solicited
that.

Perhaps you could send me your snail mail address; I'll solicit some
junk mail but put your address down. But don't call it junk mail when
you receive it, because it was solicited!

> And on the specific issue of autoresponders, I think a reasonable 
> compromise is to support DomainKeys. That way if a sender is 
> irritated 
> that they are receiving automatic responses from messages they didn't 
> send, they can personally take action to invalidate the forgery.

IMO one should never have to receive "automatic responses from
messages they didn't send".

> But mark my words: Asking hosts to stop sending bounce messages or 
> automatic responses is insane and contrary to over a decade 
> of established 
> postmaster precedent.

Things change.

..Stu

Re: bogofilter ate 3/5

[Matti Aarnio]

On Wed, Sep 06, 2006 at 02:15:46PM -0500, Chase Venters wrote:
> On Wed, 6 Sep 2006, ellis@spinics.net wrote:
> 
> >>OK, but doing something could simply consist in adding a header
> >>that anyone is free to filter on or not.
> >
> >The problem with that is the post gets no indication that his
> >mail has been filtered. The way it works now is the rejection
> >happens at SMTP time and that causes the poster to see the
> >problem. If people filtered on a header, you'd never know why you
> >weren't getting a response.
> 
> How about this:
> 
> 1. Incoming mail from subscribers is accepted
> 2. Incoming mail to honeypot addresses is trained as SPAM
> 3. Incoming mail from non-subscribers is marked with X-Bogofilter:
> 4. A handy Perl script subscribes to lkml, and for any message it gets 
> with an X-Bogofilter: SPAM header, it sends a notification (rate-limited) 
> to the message sender explaining that his message will be filtered as SPAM 
> by some recipients, and inviting him to contact postmaster to resolve the 
> issue, and additionally letting him know that notification is rate-limited 
> and there is a website he can check to see the SUBJECTs of all messages 
> filtered as SPAM on lkml (say for the last week or two) if he wants to try 
> and correct the problem himself.


Actually...

At front-door the bogofilter analyzes message for spam-signature
and does classification.  It reports the class to SMTP receiver's
content policy controller, that usually chooses to believe it.

A number of recipient addresses are considered "filter free" and
they do always get messages no matter what BF or other content filters
consider the email.

The SMTP receiver also recognizes diffs in texts (but only when
unquoted) and exempts them of BF rulings (which sometimes do bite
on diffs.)


Honeypots train BF for SPAM.

Majordomo has tons of 'TABOO' filters and any match at them
trains BF also for SPAM.

Any message that was successfully accepted to any list is
trained as HAM.


I am considering teaching the system to recognize VGER's lists
specially, and to verify that sender is in the list, or at possible
extra 'posters' list.  (Did I create one, or did I plan only ?)
Anyway, if MAIL FROM address is at either, then message is again
exempted of Bogofilter.


Presently majordomo and front-end SMTP are not in any direct contact,
and that kind of policy decissions are not made at SMTP input time.
They are done silently a bit latter.

I can move most of the Majordomo policy rules to front-end policy
controller -- it is perl, after all  :-)


> Thanks,
> Chase

/Matti Aarnio -- one of  <postmaster@vger.kernel.org>

Re: bogofilter ate 3/5

[Willy Tarreau]

On Wed, Sep 06, 2006 at 11:56:28AM -0700, ellis@spinics.net wrote:
> > OK, but doing something could simply consist in adding a header
> > that anyone is free to filter on or not.
> 
> The problem with that is the post gets no indication that his
> mail has been filtered. The way it works now is the rejection
> happens at SMTP time and that causes the poster to see the
> problem. If people filtered on a header, you'd never know why you
> weren't getting a response.

Valid point.

Regards,
Willy