From 266991b13890049ee1a6bb95b9817f06339ee3d7 Mon Sep 17 00:00:00 2001 From: Jeff Moyer Date: Mon, 20 Feb 2012 17:59:24 -0500 Subject: ext4: fix race between unwritten extent conversion and truncate The following comment in ext4_end_io_dio caught my attention: /* XXX: probably should move into the real I/O completion handler */ inode_dio_done(inode); The truncate code takes i_mutex, then calls inode_dio_wait. Because the ext4 code path above will end up dropping the mutex before it is reacquired by the worker thread that does the extent conversion, it seems to me that the truncate can happen out of order. Jan Kara mentioned that this might result in error messages in the system logs, but that should be the extent of the "damage." The fix is pretty straight-forward: don't call inode_dio_done until the extent conversion is complete. Reviewed-by: Jan Kara Signed-off-by: Jeff Moyer Signed-off-by: "Theodore Ts'o" Cc: stable@vger.kernel.org --- fs/ext4/page-io.c | 2 ++ 1 file changed, 2 insertions(+) (limited to 'fs/ext4/page-io.c') diff --git a/fs/ext4/page-io.c b/fs/ext4/page-io.c index 475851896518..9e1b8eb1e7ac 100644 --- a/fs/ext4/page-io.c +++ b/fs/ext4/page-io.c @@ -110,6 +110,8 @@ int ext4_end_io_nolock(ext4_io_end_t *io) if (io->iocb) aio_complete(io->iocb, io->result, 0); + if (io->flag & EXT4_IO_END_DIRECT) + inode_dio_done(inode); /* Wake up anyone waiting on unwritten extent conversion */ if (atomic_dec_and_test(&EXT4_I(inode)->i_aiodio_unwritten)) wake_up_all(ext4_ioend_wq(io->inode)); -- cgit v1.2.3 From 491caa43639abcffaa645fbab372a7ef4ce2975c Mon Sep 17 00:00:00 2001 From: Jeff Moyer Date: Mon, 5 Mar 2012 10:29:52 -0500 Subject: ext4: fix race between sync and completed io work The following command line will leave the aio-stress process unkillable on an ext4 file system (in my case, mounted on /mnt/test): aio-stress -t 20 -s 10 -O -S -o 2 -I 1000 /mnt/test/aiostress.3561.4 /mnt/test/aiostress.3561.4.20 /mnt/test/aiostress.3561.4.19 /mnt/test/aiostress.3561.4.18 /mnt/test/aiostress.3561.4.17 /mnt/test/aiostress.3561.4.16 /mnt/test/aiostress.3561.4.15 /mnt/test/aiostress.3561.4.14 /mnt/test/aiostress.3561.4.13 /mnt/test/aiostress.3561.4.12 /mnt/test/aiostress.3561.4.11 /mnt/test/aiostress.3561.4.10 /mnt/test/aiostress.3561.4.9 /mnt/test/aiostress.3561.4.8 /mnt/test/aiostress.3561.4.7 /mnt/test/aiostress.3561.4.6 /mnt/test/aiostress.3561.4.5 /mnt/test/aiostress.3561.4.4 /mnt/test/aiostress.3561.4.3 /mnt/test/aiostress.3561.4.2 This is using the aio-stress program from the xfstests test suite. That particular command line tells aio-stress to do random writes to 20 files from 20 threads (one thread per file). The files are NOT preallocated, so you will get writes to random offsets within the file, thus creating holes and extending i_size. It also opens the file with O_DIRECT and O_SYNC. On to the problem. When an I/O requires unwritten extent conversion, it is queued onto the completed_io_list for the ext4 inode. Two code paths will pull work items from this list. The first is the ext4_end_io_work routine, and the second is ext4_flush_completed_IO, which is called via the fsync path (and O_SYNC handling, as well). There are two issues I've found in these code paths. First, if the fsync path beats the work routine to a particular I/O, the work routine will free the io_end structure! It does not take into account the fact that the io_end may still be in use by the fsync path. I've fixed this issue by adding yet another IO_END flag, indicating that the io_end is being processed by the fsync path. The second problem is that the work routine will make an assignment to io->flag outside of the lock. I have witnessed this result in a hang at umount. Moving the flag setting inside the lock resolved that problem. The problem was introduced by commit b82e384c7b ("ext4: optimize locking for end_io extent conversion"), which first appeared in 3.2. As such, the fix should be backported to that release (probably along with the unwritten extent conversion race fix). Signed-off-by: Jeff Moyer Signed-off-by: "Theodore Ts'o" CC: stable@kernel.org --- fs/ext4/ext4.h | 1 + fs/ext4/fsync.c | 2 ++ fs/ext4/page-io.c | 9 +++++++-- 3 files changed, 10 insertions(+), 2 deletions(-) (limited to 'fs/ext4/page-io.c') diff --git a/fs/ext4/ext4.h b/fs/ext4/ext4.h index 6481e3ca3528..37e7d8b66c99 100644 --- a/fs/ext4/ext4.h +++ b/fs/ext4/ext4.h @@ -185,6 +185,7 @@ struct mpage_da_data { #define EXT4_IO_END_ERROR 0x0002 #define EXT4_IO_END_QUEUED 0x0004 #define EXT4_IO_END_DIRECT 0x0008 +#define EXT4_IO_END_IN_FSYNC 0x0010 struct ext4_io_page { struct page *p_page; diff --git a/fs/ext4/fsync.c b/fs/ext4/fsync.c index 00a2cb753efd..bb6c7d811313 100644 --- a/fs/ext4/fsync.c +++ b/fs/ext4/fsync.c @@ -89,6 +89,7 @@ int ext4_flush_completed_IO(struct inode *inode) io = list_entry(ei->i_completed_io_list.next, ext4_io_end_t, list); list_del_init(&io->list); + io->flag |= EXT4_IO_END_IN_FSYNC; /* * Calling ext4_end_io_nolock() to convert completed * IO to written. @@ -108,6 +109,7 @@ int ext4_flush_completed_IO(struct inode *inode) if (ret < 0) ret2 = ret; spin_lock_irqsave(&ei->i_completed_io_lock, flags); + io->flag &= ~EXT4_IO_END_IN_FSYNC; } spin_unlock_irqrestore(&ei->i_completed_io_lock, flags); return (ret2 < 0) ? ret2 : 0; diff --git a/fs/ext4/page-io.c b/fs/ext4/page-io.c index 9e1b8eb1e7ac..dcdeef169a69 100644 --- a/fs/ext4/page-io.c +++ b/fs/ext4/page-io.c @@ -129,12 +129,18 @@ static void ext4_end_io_work(struct work_struct *work) unsigned long flags; spin_lock_irqsave(&ei->i_completed_io_lock, flags); + if (io->flag & EXT4_IO_END_IN_FSYNC) + goto requeue; if (list_empty(&io->list)) { spin_unlock_irqrestore(&ei->i_completed_io_lock, flags); goto free; } if (!mutex_trylock(&inode->i_mutex)) { + bool was_queued; +requeue: + was_queued = !!(io->flag & EXT4_IO_END_QUEUED); + io->flag |= EXT4_IO_END_QUEUED; spin_unlock_irqrestore(&ei->i_completed_io_lock, flags); /* * Requeue the work instead of waiting so that the work @@ -147,9 +153,8 @@ static void ext4_end_io_work(struct work_struct *work) * yield the cpu if it sees an end_io request that has already * been requeued. */ - if (io->flag & EXT4_IO_END_QUEUED) + if (was_queued) yield(); - io->flag |= EXT4_IO_END_QUEUED; return; } list_del_init(&io->list); -- cgit v1.2.3 From b43d17f319f2c502b17139d1cf70731b2b62c644 Mon Sep 17 00:00:00 2001 From: Curt Wohlgemuth Date: Mon, 5 Mar 2012 10:40:15 -0500 Subject: ext4: don't release page refs in ext4_end_bio() We can clear PageWriteback on each page when the IO completes, but we can't release the references on the page until we convert any uninitialized extents. Without this patch, the use of the dioread_nolock mount option can break buffered writes, because extents may not be converted by the time a subsequent buffered read comes in; if the page is not in the page cache, a read will return zeros if the extent is still uninitialized. I tested this with a (temporary) patch that adds a call to msleep(1000) at the start of ext4_end_io_work(), to delay processing of each DIO-unwritten work queue item. With this msleep(), a simple workload of fallocate write fadvise read will fail without this patch, succeeds with it. Signed-off-by: Curt Wohlgemuth Signed-off-by: "Theodore Ts'o" --- fs/ext4/page-io.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) (limited to 'fs/ext4/page-io.c') diff --git a/fs/ext4/page-io.c b/fs/ext4/page-io.c index dcdeef169a69..74cd1f7f1f88 100644 --- a/fs/ext4/page-io.c +++ b/fs/ext4/page-io.c @@ -60,7 +60,6 @@ void ext4_ioend_wait(struct inode *inode) static void put_io_page(struct ext4_io_page *io_page) { if (atomic_dec_and_test(&io_page->p_count)) { - end_page_writeback(io_page->p_page); put_page(io_page->p_page); kmem_cache_free(io_page_cachep, io_page); } @@ -234,9 +233,9 @@ static void ext4_end_bio(struct bio *bio, int error) } while (bh != head); } - put_io_page(io_end->pages[i]); + if (atomic_read(&io_end->pages[i]->p_count) == 1) + end_page_writeback(io_end->pages[i]->p_page); } - io_end->num_io_pages = 0; inode = io_end->inode; if (error) { @@ -428,6 +427,8 @@ int ext4_bio_write_page(struct ext4_io_submit *io, * PageWriteback bit from the page to prevent the system from * wedging later on. */ + if (atomic_read(&io_page->p_count) == 1) + end_page_writeback(page); put_io_page(io_page); return ret; } -- cgit v1.2.3 From 6268b325c3066234e7bddb99d2b98bcedb0c0033 Mon Sep 17 00:00:00 2001 From: Linus Torvalds Date: Thu, 29 Mar 2012 17:00:56 -0700 Subject: Revert "ext4: don't release page refs in ext4_end_bio()" This reverts commit b43d17f319f2c502b17139d1cf70731b2b62c644. Dave Jones reports that it causes lockups on his laptop, and his debug output showed a lot of processes hung waiting for page_writeback (or more commonly - processes hung waiting for a lock that was held during that writeback wait). The page_writeback hint made Ted suggest that Dave look at this commit, and Dave verified that reverting it makes his problems go away. Ted says: "That commit fixes a race which is seen when you write into fallocated (and hence uninitialized) disk blocks under *very* heavy memory pressure. Furthermore, although theoretically it could trigger under normal direct I/O writes, it only seems to trigger if you are issuing a huge number of AIO writes, such that a just-written page can get evicted from memory, and then read back into memory, before the workqueue has a chance to update the extent tree. This race has been around for a little over a year, and no one noticed until two months ago; it only happens under fairly exotic conditions, and in fact even after trying very hard to create a simple repro under lab conditions, we could only reproduce the problem and confirm the fix on production servers running MySQL on very fast PCIe-attached flash devices. Given that Dave was able to hit this problem pretty quickly, if we confirm that this commit is at fault, the only reasonable thing to do is to revert it IMO." Reported-and-tested-by: Dave Jones Acked-by: Theodore Ts'o Signed-off-by: Linus Torvalds --- fs/ext4/page-io.c | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) (limited to 'fs/ext4/page-io.c') diff --git a/fs/ext4/page-io.c b/fs/ext4/page-io.c index 74cd1f7f1f88..dcdeef169a69 100644 --- a/fs/ext4/page-io.c +++ b/fs/ext4/page-io.c @@ -60,6 +60,7 @@ void ext4_ioend_wait(struct inode *inode) static void put_io_page(struct ext4_io_page *io_page) { if (atomic_dec_and_test(&io_page->p_count)) { + end_page_writeback(io_page->p_page); put_page(io_page->p_page); kmem_cache_free(io_page_cachep, io_page); } @@ -233,9 +234,9 @@ static void ext4_end_bio(struct bio *bio, int error) } while (bh != head); } - if (atomic_read(&io_end->pages[i]->p_count) == 1) - end_page_writeback(io_end->pages[i]->p_page); + put_io_page(io_end->pages[i]); } + io_end->num_io_pages = 0; inode = io_end->inode; if (error) { @@ -427,8 +428,6 @@ int ext4_bio_write_page(struct ext4_io_submit *io, * PageWriteback bit from the page to prevent the system from * wedging later on. */ - if (atomic_read(&io_page->p_count) == 1) - end_page_writeback(page); put_io_page(io_page); return ret; } -- cgit v1.2.3