Hi,
this is a second version of my patches which rewrite ext4_page_mkwrite() to
use generic helper. When we are already changing this code, I also added proper
handling of frozen filesystem there in patch 2/3. I've incorporated suggestions
from Christoph in the patches which resulted in some patch reorg. I'd like to
get these patches reviewed and then get them merged possibly via ext4-tree
(although the first two patches might go via Al but that would complicate
merging of ext4 part where I expect more conflicts with ongoing work).
Honza
We should not allow file modification via mmap while the filesystem is
frozen. So block in block_page_mkwrite() while the filesystem is frozen.
We cannot do the blocking wait in __block_page_mkwrite() since e.g. ext4
will want to call that function with transaction started in some cases
and that would deadlock. But we can at least do the non-blocking reliable
check in __block_page_mkwrite() which is the hardest part anyway.
We have to check for frozen filesystem with the page marked dirty and under
page lock with which we then return from ->page_mkwrite(). Only that way we
cannot race with writeback done by freezing code - either we mark the page
dirty after the writeback has started, see freezing in progress and block, or
writeback will wait for our page lock which is released only when the fault is
done and then writeback will writeout and writeprotect the page again.
Signed-off-by: Jan Kara <[email protected]>
---
fs/buffer.c | 28 +++++++++++++++++++++++++++-
include/linux/buffer_head.h | 2 ++
2 files changed, 29 insertions(+), 1 deletions(-)
diff --git a/fs/buffer.c b/fs/buffer.c
index 9c5dd88..030f808 100644
--- a/fs/buffer.c
+++ b/fs/buffer.c
@@ -2331,6 +2331,9 @@ EXPORT_SYMBOL(block_commit_write);
* page lock we can determine safely if the page is beyond EOF. If it is not
* beyond EOF, then the page is guaranteed safe against truncation until we
* unlock the page.
+ *
+ * Direct callers of this function should call vfs_check_frozen() so that page
+ * fault does not busyloop until the fs is thawed.
*/
int __block_page_mkwrite(struct vm_area_struct *vma, struct vm_fault *vmf,
get_block_t get_block)
@@ -2363,6 +2366,22 @@ int __block_page_mkwrite(struct vm_area_struct *vma, struct vm_fault *vmf,
if (unlikely(ret < 0))
unlock_page(page);
+ else {
+ /*
+ * Freezing in progress? We check after the page is marked
+ * dirty and with page lock held so if the test here fails, we
+ * are sure freezing code will wait during syncing until the
+ * page fault is done - at that point page will be dirty and
+ * unlocked so freezing code will write it and writeprotect it
+ * again.
+ */
+ set_page_dirty(page);
+ if (inode->i_sb->s_frozen != SB_UNFROZEN) {
+ unlock_page(page);
+ ret = -EAGAIN;
+ goto out;
+ }
+ }
out:
return ret;
}
@@ -2371,8 +2390,15 @@ EXPORT_SYMBOL(__block_page_mkwrite);
int block_page_mkwrite(struct vm_area_struct *vma, struct vm_fault *vmf,
get_block_t get_block)
{
- int ret = __block_page_mkwrite(vma, vmf, get_block);
+ int ret;
+ struct super_block *sb = vma->vm_file->f_path.dentry->d_inode->i_sb;
+ /*
+ * This check is racy but catches the common case. The check in
+ * __block_page_mkwrite() is reliable.
+ */
+ vfs_check_frozen(sb, SB_FREEZE_WRITE);
+ ret = __block_page_mkwrite(vma, vmf, get_block);
return block_page_mkwrite_return(ret);
}
EXPORT_SYMBOL(block_page_mkwrite);
diff --git a/include/linux/buffer_head.h b/include/linux/buffer_head.h
index 2bf6a91..503c8a6 100644
--- a/include/linux/buffer_head.h
+++ b/include/linux/buffer_head.h
@@ -230,6 +230,8 @@ static inline int block_page_mkwrite_return(int err)
return VM_FAULT_NOPAGE;
if (err == -ENOMEM)
return VM_FAULT_OOM;
+ if (err == -EAGAIN)
+ return VM_FAULT_RETRY;
/* -ENOSPC, -EDQUOT, -EIO ... */
return VM_FAULT_SIGBUS;
}
--
1.7.1
Create __block_page_mkwrite() helper which does all what block_page_mkwrite()
does except that it passes back errors from __block_write_begin /
block_commit_write calls.
Signed-off-by: Jan Kara <[email protected]>
---
fs/buffer.c | 28 +++++++++++++++-------------
include/linux/buffer_head.h | 14 ++++++++++++++
2 files changed, 29 insertions(+), 13 deletions(-)
diff --git a/fs/buffer.c b/fs/buffer.c
index a08bb8e..9c5dd88 100644
--- a/fs/buffer.c
+++ b/fs/buffer.c
@@ -2332,21 +2332,21 @@ EXPORT_SYMBOL(block_commit_write);
* beyond EOF, then the page is guaranteed safe against truncation until we
* unlock the page.
*/
-int
-block_page_mkwrite(struct vm_area_struct *vma, struct vm_fault *vmf,
- get_block_t get_block)
+int __block_page_mkwrite(struct vm_area_struct *vma, struct vm_fault *vmf,
+ get_block_t get_block)
{
struct page *page = vmf->page;
struct inode *inode = vma->vm_file->f_path.dentry->d_inode;
unsigned long end;
loff_t size;
- int ret = VM_FAULT_NOPAGE; /* make the VM retry the fault */
+ int ret = 0;
lock_page(page);
size = i_size_read(inode);
if ((page->mapping != inode->i_mapping) ||
(page_offset(page) > size)) {
- /* page got truncated out from underneath us */
+ /* We overload EFAULT to mean page got truncated */
+ ret = -EFAULT;
unlock_page(page);
goto out;
}
@@ -2361,18 +2361,20 @@ block_page_mkwrite(struct vm_area_struct *vma, struct vm_fault *vmf,
if (!ret)
ret = block_commit_write(page, 0, end);
- if (unlikely(ret)) {
+ if (unlikely(ret < 0))
unlock_page(page);
- if (ret == -ENOMEM)
- ret = VM_FAULT_OOM;
- else /* -ENOSPC, -EIO, etc */
- ret = VM_FAULT_SIGBUS;
- } else
- ret = VM_FAULT_LOCKED;
-
out:
return ret;
}
+EXPORT_SYMBOL(__block_page_mkwrite);
+
+int block_page_mkwrite(struct vm_area_struct *vma, struct vm_fault *vmf,
+ get_block_t get_block)
+{
+ int ret = __block_page_mkwrite(vma, vmf, get_block);
+
+ return block_page_mkwrite_return(ret);
+}
EXPORT_SYMBOL(block_page_mkwrite);
/*
diff --git a/include/linux/buffer_head.h b/include/linux/buffer_head.h
index f5df235..2bf6a91 100644
--- a/include/linux/buffer_head.h
+++ b/include/linux/buffer_head.h
@@ -217,8 +217,22 @@ int cont_write_begin(struct file *, struct address_space *, loff_t,
get_block_t *, loff_t *);
int generic_cont_expand_simple(struct inode *inode, loff_t size);
int block_commit_write(struct page *page, unsigned from, unsigned to);
+int __block_page_mkwrite(struct vm_area_struct *vma, struct vm_fault *vmf,
+ get_block_t get_block);
int block_page_mkwrite(struct vm_area_struct *vma, struct vm_fault *vmf,
get_block_t get_block);
+/* Convert errno to return value from ->page_mkwrite() call */
+static inline int block_page_mkwrite_return(int err)
+{
+ if (err == 0)
+ return VM_FAULT_LOCKED;
+ if (err == -EFAULT)
+ return VM_FAULT_NOPAGE;
+ if (err == -ENOMEM)
+ return VM_FAULT_OOM;
+ /* -ENOSPC, -EDQUOT, -EIO ... */
+ return VM_FAULT_SIGBUS;
+}
sector_t generic_block_bmap(struct address_space *, sector_t, get_block_t *);
int block_truncate_page(struct address_space *, loff_t, get_block_t *);
int nobh_write_begin(struct address_space *, loff_t, unsigned, unsigned,
--
1.7.1
ext4_page_mkwrite() does not return page locked. This makes it hard
to avoid races with filesystem freezing code (so that we don't leave
writeable page on a frozen fs) or writeback code (so that we allow page
to be stable during writeback).
Also the current code uses i_alloc_sem to avoid races with truncate but that
seems to be the wrong locking order according to lock ordering documented in
mm/rmap.c.
Also add a check for frozen filesystem so that we don't busyloop in page fault
when the filesystem is frozen.
Signed-off-by: Jan Kara <[email protected]>
---
fs/ext4/inode.c | 94 ++++++++++++++++++++++++++++++++-----------------------
1 files changed, 55 insertions(+), 39 deletions(-)
diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
index f2fa5e8..3e7b6b6 100644
--- a/fs/ext4/inode.c
+++ b/fs/ext4/inode.c
@@ -5793,66 +5793,82 @@ int ext4_page_mkwrite(struct vm_area_struct *vma, struct vm_fault *vmf)
struct page *page = vmf->page;
loff_t size;
unsigned long len;
- int ret = -EINVAL;
- void *fsdata;
+ int ret;
struct file *file = vma->vm_file;
struct inode *inode = file->f_path.dentry->d_inode;
struct address_space *mapping = inode->i_mapping;
+ handle_t *handle;
+ get_block_t *get_block;
+ int retries = 0;
/*
- * Get i_alloc_sem to stop truncates messing with the inode. We cannot
- * get i_mutex because we are already holding mmap_sem.
+ * This check is racy but catches the common case. We rely on
+ * __block_page_mkwrite() to do a reliable check.
*/
- down_read(&inode->i_alloc_sem);
+ vfs_check_frozen(inode->i_sb, SB_FREEZE_WRITE);
+ /* Delalloc case is easy... */
+ if (test_opt(inode->i_sb, DELALLOC) &&
+ !ext4_should_journal_data(inode) &&
+ !ext4_nonda_switch(inode->i_sb)) {
+ do {
+ ret = __block_page_mkwrite(vma, vmf,
+ ext4_da_get_block_prep);
+ } while (ret == -ENOSPC &&
+ ext4_should_retry_alloc(inode->i_sb, &retries));
+ goto out_ret;
+ }
+
+ lock_page(page);
size = i_size_read(inode);
- if (page->mapping != mapping || size <= page_offset(page)
- || !PageUptodate(page)) {
- /* page got truncated from under us? */
- goto out_unlock;
+ /* Page got truncated from under us? */
+ if (page->mapping != mapping || page_offset(page) > size) {
+ unlock_page(page);
+ ret = VM_FAULT_NOPAGE;
+ goto out;
}
- ret = 0;
- if (PageMappedToDisk(page))
- goto out_unlock;
if (page->index == size >> PAGE_CACHE_SHIFT)
len = size & ~PAGE_CACHE_MASK;
else
len = PAGE_CACHE_SIZE;
-
- lock_page(page);
/*
- * return if we have all the buffers mapped. This avoid
- * the need to call write_begin/write_end which does a
- * journal_start/journal_stop which can block and take
- * long time
+ * Return if we have all the buffers mapped. This avoids the need to do
+ * journal_start/journal_stop which can block and take a long time
*/
if (page_has_buffers(page)) {
if (!walk_page_buffers(NULL, page_buffers(page), 0, len, NULL,
ext4_bh_unmapped)) {
- unlock_page(page);
- goto out_unlock;
+ ret = VM_FAULT_LOCKED;
+ goto out;
}
}
unlock_page(page);
- /*
- * OK, we need to fill the hole... Do write_begin write_end
- * to do block allocation/reservation.We are not holding
- * inode.i__mutex here. That allow * parallel write_begin,
- * write_end call. lock_page prevent this from happening
- * on the same page though
- */
- ret = mapping->a_ops->write_begin(file, mapping, page_offset(page),
- len, AOP_FLAG_UNINTERRUPTIBLE, &page, &fsdata);
- if (ret < 0)
- goto out_unlock;
- ret = mapping->a_ops->write_end(file, mapping, page_offset(page),
- len, len, page, fsdata);
- if (ret < 0)
- goto out_unlock;
- ret = 0;
-out_unlock:
- if (ret)
+ /* OK, we need to fill the hole... */
+ if (ext4_should_dioread_nolock(inode))
+ get_block = ext4_get_block_write;
+ else
+ get_block = ext4_get_block;
+retry_alloc:
+ handle = ext4_journal_start(inode, ext4_writepage_trans_blocks(inode));
+ if (IS_ERR(handle)) {
ret = VM_FAULT_SIGBUS;
- up_read(&inode->i_alloc_sem);
+ goto out;
+ }
+ ret = __block_page_mkwrite(vma, vmf, get_block);
+ if (!ret && ext4_should_journal_data(inode)) {
+ if (walk_page_buffers(handle, page_buffers(page), 0,
+ PAGE_CACHE_SIZE, NULL, do_journal_get_write_access)) {
+ unlock_page(page);
+ ret = VM_FAULT_SIGBUS;
+ goto out;
+ }
+ ext4_set_inode_state(inode, EXT4_STATE_JDATA);
+ }
+ ext4_journal_stop(handle);
+ if (ret == -ENOSPC && ext4_should_retry_alloc(inode->i_sb, &retries))
+ goto retry_alloc;
+out_ret:
+ ret = block_page_mkwrite_return(ret);
+out:
return ret;
}
--
1.7.1
On Wed, May 18, 2011 at 05:17:59PM +0200, Jan Kara wrote:
>
> Hi,
>
> this is a second version of my patches which rewrite ext4_page_mkwrite() to
> use generic helper. When we are already changing this code, I also added proper
> handling of frozen filesystem there in patch 2/3. I've incorporated suggestions
> from Christoph in the patches which resulted in some patch reorg. I'd like to
> get these patches reviewed and then get them merged possibly via ext4-tree
> (although the first two patches might go via Al but that would complicate
> merging of ext4 part where I expect more conflicts with ongoing work).
Do you want me to generate a patch to add wait_on_page_writeback to the
appropriate place(s) in your new *_page_mkwrite functions, or do you plan to
add them yourself?
(iow: is there an order for pushing these ext4_page_mkwrite changes?)
--D
On Wed, May 18, 2011 at 05:18:00PM +0200, Jan Kara wrote:
> Create __block_page_mkwrite() helper which does all what block_page_mkwrite()
> does except that it passes back errors from __block_write_begin /
> block_commit_write calls.
Looks good to me. Long term I wonder wether we should force
->page_mkwrite to always return the page locked and just use normal
errnos all the way up to do_wp_page, but that can be left for a later
iteration as it would be quite invasive.
Reviewed-by: Christoph Hellwig <[email protected]>
>
> if (unlikely(ret < 0))
> unlock_page(page);
> + else {
> + /*
> + * Freezing in progress? We check after the page is marked
> + * dirty and with page lock held so if the test here fails, we
> + * are sure freezing code will wait during syncing until the
> + * page fault is done - at that point page will be dirty and
> + * unlocked so freezing code will write it and writeprotect it
> + * again.
> + */
> + set_page_dirty(page);
> + if (inode->i_sb->s_frozen != SB_UNFROZEN) {
> + unlock_page(page);
> + ret = -EAGAIN;
> + goto out;
> + }
> + }
> out:
> return ret;
The code structure looks a bit odd, why not:
if (ret < 0)
goto out_unlock;
set_page_dirty(page);
if (inode->i_sb->s_frozen != SB_UNFROZEN) {
ret = -EAGAIN;
goto out_unlock;
}
return 0;
out_unlock:
unlock_page(page);
return ret;
}
Otherwise looks good,
Reviewed-by: Christoph Hellwig <[email protected]>
On Wed, May 18, 2011 at 05:17:59PM +0200, Jan Kara wrote:
> from Christoph in the patches which resulted in some patch reorg. I'd like to
> get these patches reviewed and then get them merged possibly via ext4-tree
> (although the first two patches might go via Al but that would complicate
> merging of ext4 part where I expect more conflicts with ongoing work).
The core canges are clear VFS tree material, and given that they are
needed for the stable pages patches as well it really doesn't make sense
to funnel this through a fs tree. It might make sense to include the
ext4 patch in the VFS pile if that makes merging easier.
On Wed 18-05-11 11:00:22, Darrick J. Wong wrote:
> On Wed, May 18, 2011 at 05:17:59PM +0200, Jan Kara wrote:
> >
> > Hi,
> >
> > this is a second version of my patches which rewrite ext4_page_mkwrite() to
> > use generic helper. When we are already changing this code, I also added proper
> > handling of frozen filesystem there in patch 2/3. I've incorporated suggestions
> > from Christoph in the patches which resulted in some patch reorg. I'd like to
> > get these patches reviewed and then get them merged possibly via ext4-tree
> > (although the first two patches might go via Al but that would complicate
> > merging of ext4 part where I expect more conflicts with ongoing work).
>
> Do you want me to generate a patch to add wait_on_page_writeback to the
> appropriate place(s) in your new *_page_mkwrite functions, or do you plan to
> add them yourself?
Since Ted has merged your stable pages patches touching ext4, I'll rebase
my ext4 patch on top of that. Regarding block_page_mkwrite() I will add
that one-liner to the series (because the ext4_page_mkwrite() now depends
on that).
Honza
On Wed 18-05-11 14:12:06, Christoph Hellwig wrote:
> >
> > if (unlikely(ret < 0))
> > unlock_page(page);
> > + else {
> > + /*
> > + * Freezing in progress? We check after the page is marked
> > + * dirty and with page lock held so if the test here fails, we
> > + * are sure freezing code will wait during syncing until the
> > + * page fault is done - at that point page will be dirty and
> > + * unlocked so freezing code will write it and writeprotect it
> > + * again.
> > + */
> > + set_page_dirty(page);
> > + if (inode->i_sb->s_frozen != SB_UNFROZEN) {
> > + unlock_page(page);
> > + ret = -EAGAIN;
> > + goto out;
> > + }
> > + }
> > out:
> > return ret;
>
> The code structure looks a bit odd, why not:
>
> if (ret < 0)
> goto out_unlock;
>
> set_page_dirty(page);
> if (inode->i_sb->s_frozen != SB_UNFROZEN) {
> ret = -EAGAIN;
> goto out_unlock;
> }
>
> return 0;
>
> out_unlock:
> unlock_page(page);
> return ret;
> }
>
> Otherwise looks good,
>
> Reviewed-by: Christoph Hellwig <[email protected]>
Thanks, I've changed the flow as you suggested.
Honza
On Wed 18-05-11 14:13:21, Christoph Hellwig wrote:
> On Wed, May 18, 2011 at 05:17:59PM +0200, Jan Kara wrote:
> > from Christoph in the patches which resulted in some patch reorg. I'd like to
> > get these patches reviewed and then get them merged possibly via ext4-tree
> > (although the first two patches might go via Al but that would complicate
> > merging of ext4 part where I expect more conflicts with ongoing work).
>
> The core canges are clear VFS tree material, and given that they are
> needed for the stable pages patches as well it really doesn't make sense
> to funnel this through a fs tree. It might make sense to include the
> ext4 patch in the VFS pile if that makes merging easier.
OK, I'll merge the first two patches via Al.
Honza
On Wed 18-05-11 11:00:22, Darrick J. Wong wrote:
> On Wed, May 18, 2011 at 05:17:59PM +0200, Jan Kara wrote:
> >
> > Hi,
> >
> > this is a second version of my patches which rewrite ext4_page_mkwrite() to
> > use generic helper. When we are already changing this code, I also added proper
> > handling of frozen filesystem there in patch 2/3. I've incorporated suggestions
> > from Christoph in the patches which resulted in some patch reorg. I'd like to
> > get these patches reviewed and then get them merged possibly via ext4-tree
> > (although the first two patches might go via Al but that would complicate
> > merging of ext4 part where I expect more conflicts with ongoing work).
>
> Do you want me to generate a patch to add wait_on_page_writeback to the
> appropriate place(s) in your new *_page_mkwrite functions, or do you plan to
> add them yourself?
>
> (iow: is there an order for pushing these ext4_page_mkwrite changes?)
Attached is a rebase of your block_page_mkwrite change on top of my
patches. I'm not sure what is the actual status your series - have you
pushed VFS changes to Al?
Honza
On Thu, May 19, 2011 at 02:30:29PM +0200, Jan Kara wrote:
> On Wed 18-05-11 11:00:22, Darrick J. Wong wrote:
> > On Wed, May 18, 2011 at 05:17:59PM +0200, Jan Kara wrote:
> > >
> > > Hi,
> > >
> > > this is a second version of my patches which rewrite ext4_page_mkwrite() to
> > > use generic helper. When we are already changing this code, I also added proper
> > > handling of frozen filesystem there in patch 2/3. I've incorporated suggestions
> > > from Christoph in the patches which resulted in some patch reorg. I'd like to
> > > get these patches reviewed and then get them merged possibly via ext4-tree
> > > (although the first two patches might go via Al but that would complicate
> > > merging of ext4 part where I expect more conflicts with ongoing work).
> >
> > Do you want me to generate a patch to add wait_on_page_writeback to the
> > appropriate place(s) in your new *_page_mkwrite functions, or do you plan to
> > add them yourself?
> >
> > (iow: is there an order for pushing these ext4_page_mkwrite changes?)
> Attached is a rebase of your block_page_mkwrite change on top of my
> patches. I'm not sure what is the actual status your series - have you
> pushed VFS changes to Al?
Yes, the previous set of patches was sent to Al. I guess I should resend
without the parts that have gone upstream or aren't really needed.
--D
>
> Honza
> From 408d0a667ebdc7edd24d05c8de4b42cc1629e84f Mon Sep 17 00:00:00 2001
> From: Darrick J. Wong <[email protected]>
> Date: Thu, 19 May 2011 14:14:11 +0200
> Subject: [PATCH] vfs: Wait in __block_page_mkwrite for IO to finish
>
> For filesystems such as nilfs2 and xfs that use block_page_mkwrite, modify that
> function to wait for pending writeback before allowing the page to become
> writable. This is needed to stabilize pages during writeback for those two
> filesystems.
>
> Signed-off-by: Darrick J. Wong <[email protected]>
> Signed-off-by: Jan Kara <[email protected]>
> ---
> fs/buffer.c | 2 ++
> 1 files changed, 2 insertions(+), 0 deletions(-)
>
> diff --git a/fs/buffer.c b/fs/buffer.c
> index b0675bf..161685d 100644
> --- a/fs/buffer.c
> +++ b/fs/buffer.c
> @@ -2365,6 +2365,8 @@ int __block_page_mkwrite(struct vm_area_struct *vma, struct vm_fault *vmf,
>
> if (unlikely(ret < 0))
> goto out_unlock;
> + /* Wait so that we don't change page under IO */
> + wait_on_page_writeback(page);
> /*
> * Freezing in progress? We check after the page is marked dirty and
> * with page lock held so if the test here fails, we are sure freezing
> --
> 1.7.1
>
On Thu, May 19, 2011 at 12:57:08PM +0200, Jan Kara wrote:
> Since Ted has merged your stable pages patches touching ext4, I'll rebase
> my ext4 patch on top of that. Regarding block_page_mkwrite() I will add
> that one-liner to the series (because the ext4_page_mkwrite() now depends
> on that).
Hi Jan,
Does your 3/3 ext4 patch have dependencies on your 1/3 and 2/3
VFS-layer patches?
Thanks,
- Ted
On Fri 20-05-11 13:59:53, Ted Tso wrote:
> On Thu, May 19, 2011 at 12:57:08PM +0200, Jan Kara wrote:
> > Since Ted has merged your stable pages patches touching ext4, I'll rebase
> > my ext4 patch on top of that. Regarding block_page_mkwrite() I will add
> > that one-liner to the series (because the ext4_page_mkwrite() now depends
> > on that).
Hi Ted,
> Does your 3/3 ext4 patch have dependencies on your 1/3 and 2/3
> VFS-layer patches?
Yes, it does depend on the new helper introduced in 1/3. Should I push
3/3 via Al as well or just wait with it until Al merges the first two
patches and then push it to you?
Honza
--
Jan Kara <[email protected]>
SUSE Labs, CR
On Mon, May 23, 2011 at 03:21:58PM +0200, Jan Kara wrote:
> > Does your 3/3 ext4 patch have dependencies on your 1/3 and 2/3
> > VFS-layer patches?
> Yes, it does depend on the new helper introduced in 1/3. Should I push
> 3/3 via Al as well or just wait with it until Al merges the first two
> patches and then push it to you?
Hmm... I guess it depends on how quickly Al is going to be pushing
VFS changes to Linus (I assume you're hoping this is going to get
pushed during this merge window, yes?). I still have at least a day
or two worth of final merging and testing to do, and your 3/3 ext4
patch has a dependency on Darrick's T10 DIF/DIX patch as I recall.
- Ted