From: "Lukáš Czerner" <lczerner@redhat.com> To: Ashish Sangwan <ashishsangwan2@gmail.com> Cc: Lukas Czerner <lczerner@redhat.com>, linux-fsdevel@vger.kernel.org, linux-ext4@vger.kernel.org, tytso@mit.edu, hughd@google.com, linux-mm@kvack.org Subject: Re: [PATCH 07/15 v2] ext4: Take i_mutex before punching hole Date: Thu, 13 Sep 2012 11:15:03 -0400 (EDT) [thread overview] Message-ID: <alpine.LFD.2.00.1209131113450.15781@dhcp-196-88.bos.redhat.com> (raw) In-Reply-To: <CAOiN93kKVxYeS5f0_nR3RpdX7sv+EJNA-T4jq7amFS5LQGqfnw@mail.gmail.com> On Mon, 10 Sep 2012, Ashish Sangwan wrote: > Date: Mon, 10 Sep 2012 17:30:53 +0530 > From: Ashish Sangwan <ashishsangwan2@gmail.com> > To: Lukas Czerner <lczerner@redhat.com> > Cc: linux-fsdevel@vger.kernel.org, linux-ext4@vger.kernel.org, tytso@mit.edu, > hughd@google.com, linux-mm@kvack.org > Subject: Re: [PATCH 07/15 v2] ext4: Take i_mutex before punching hole > > On Sat, Sep 1, 2012 at 3:51 AM, Lukas Czerner <lczerner@redhat.com> wrote: > > Currently the allocation might happen in the punched range after the > > truncation and before the releasing the space of the range. This would > > lead to blocks being unallocated under the mapped buffer heads resulting > > in nasty bugs. > > > > With this commit we take i_mutex before going to do anything in the > > ext4_ext_punch_hole() preventing any write to happen while the hole > > punching is in progress. This will also allow us to ditch the writeout > > of dirty pages withing the range. > > > > This commit was based on code provided by Zheng Liu, thanks! > > > > Signed-off-by: Lukas Czerner <lczerner@redhat.com> > > --- > > fs/ext4/extents.c | 26 ++++++++++---------------- > > 1 files changed, 10 insertions(+), 16 deletions(-) > > > > diff --git a/fs/ext4/extents.c b/fs/ext4/extents.c > > index aabbb3f..f920383 100644 > > --- a/fs/ext4/extents.c > > +++ b/fs/ext4/extents.c > > @@ -4769,9 +4769,11 @@ int ext4_ext_punch_hole(struct file *file, loff_t offset, loff_t length) > > loff_t first_page_offset, last_page_offset; > > int credits, err = 0; > > > > + mutex_lock(&inode->i_mutex); > > + > > /* No need to punch hole beyond i_size */ > > if (offset >= inode->i_size) > > - return 0; > > + goto out1; > > > > /* > > * If the hole extends beyond i_size, set the hole > > @@ -4789,18 +4791,6 @@ int ext4_ext_punch_hole(struct file *file, loff_t offset, loff_t length) > > first_page_offset = first_page << PAGE_CACHE_SHIFT; > > last_page_offset = last_page << PAGE_CACHE_SHIFT; > > > > - /* > > - * Write out all dirty pages to avoid race conditions > > - * Then release them. > > - */ > > - if (mapping->nrpages && mapping_tagged(mapping, PAGECACHE_TAG_DIRTY)) { > > - err = filemap_write_and_wait_range(mapping, > > - offset, offset + length - 1); > > - > > - if (err) > > - return err; > > - } > > - > > Removing above code will cause a problem in case the file has all its > data in memory and nothing has been committed on disk. If punch hole > is issued for such a file, as there are no extents present, EIO would > be returned from ext4_ext_rm_leaf. So, even though blocks would be > removed from memory, the end result will be error EIO. > > > /* Now release the pages */ > > if (last_page_offset > first_page_offset) { > > truncate_pagecache_range(inode, first_page_offset, > > To avoid this, you can add a check after the call to truncate_pagecache_range. > if(!inode->i_blocks) > return 0; Thanks for pointing this out. However Dimitry has better fix for this with some additional changes so I am dropping this particular patch. (see "ext4: punch_hole should wait for DIO writers") Thanks! -Lukas > > > @@ -4812,12 +4802,14 @@ int ext4_ext_punch_hole(struct file *file, loff_t offset, loff_t length) > > > > credits = ext4_writepage_trans_blocks(inode); > > handle = ext4_journal_start(inode, credits); > > - if (IS_ERR(handle)) > > - return PTR_ERR(handle); > > + if (IS_ERR(handle)) { > > + err = PTR_ERR(handle); > > + goto out1; > > + } > > > > err = ext4_orphan_add(handle, inode); > > if (err) > > - goto out; > > + goto out1; > > > > /* > > * Now we need to zero out the non-page-aligned data in the > > @@ -4907,6 +4899,8 @@ out: > > inode->i_mtime = inode->i_ctime = ext4_current_time(inode); > > ext4_mark_inode_dirty(handle, inode); > > ext4_journal_stop(handle); > > +out1: > > + mutex_unlock(&inode->i_mutex); > > return err; > > } > > int ext4_fiemap(struct inode *inode, struct fiemap_extent_info *fieinfo, > > -- > > 1.7.7.6 > > > > -- > > To unsubscribe from this list: send the line "unsubscribe linux-ext4" in > > the body of a message to majordomo@vger.kernel.org > > More majordomo info at http://vger.kernel.org/majordomo-info.html >
WARNING: multiple messages have this Message-ID (diff)
From: "Lukáš Czerner" <lczerner@redhat.com> To: Ashish Sangwan <ashishsangwan2@gmail.com> Cc: Lukas Czerner <lczerner@redhat.com>, linux-fsdevel@vger.kernel.org, linux-ext4@vger.kernel.org, tytso@mit.edu, hughd@google.com, linux-mm@kvack.org Subject: Re: [PATCH 07/15 v2] ext4: Take i_mutex before punching hole Date: Thu, 13 Sep 2012 11:15:03 -0400 (EDT) [thread overview] Message-ID: <alpine.LFD.2.00.1209131113450.15781@dhcp-196-88.bos.redhat.com> (raw) In-Reply-To: <CAOiN93kKVxYeS5f0_nR3RpdX7sv+EJNA-T4jq7amFS5LQGqfnw@mail.gmail.com> On Mon, 10 Sep 2012, Ashish Sangwan wrote: > Date: Mon, 10 Sep 2012 17:30:53 +0530 > From: Ashish Sangwan <ashishsangwan2@gmail.com> > To: Lukas Czerner <lczerner@redhat.com> > Cc: linux-fsdevel@vger.kernel.org, linux-ext4@vger.kernel.org, tytso@mit.edu, > hughd@google.com, linux-mm@kvack.org > Subject: Re: [PATCH 07/15 v2] ext4: Take i_mutex before punching hole > > On Sat, Sep 1, 2012 at 3:51 AM, Lukas Czerner <lczerner@redhat.com> wrote: > > Currently the allocation might happen in the punched range after the > > truncation and before the releasing the space of the range. This would > > lead to blocks being unallocated under the mapped buffer heads resulting > > in nasty bugs. > > > > With this commit we take i_mutex before going to do anything in the > > ext4_ext_punch_hole() preventing any write to happen while the hole > > punching is in progress. This will also allow us to ditch the writeout > > of dirty pages withing the range. > > > > This commit was based on code provided by Zheng Liu, thanks! > > > > Signed-off-by: Lukas Czerner <lczerner@redhat.com> > > --- > > fs/ext4/extents.c | 26 ++++++++++---------------- > > 1 files changed, 10 insertions(+), 16 deletions(-) > > > > diff --git a/fs/ext4/extents.c b/fs/ext4/extents.c > > index aabbb3f..f920383 100644 > > --- a/fs/ext4/extents.c > > +++ b/fs/ext4/extents.c > > @@ -4769,9 +4769,11 @@ int ext4_ext_punch_hole(struct file *file, loff_t offset, loff_t length) > > loff_t first_page_offset, last_page_offset; > > int credits, err = 0; > > > > + mutex_lock(&inode->i_mutex); > > + > > /* No need to punch hole beyond i_size */ > > if (offset >= inode->i_size) > > - return 0; > > + goto out1; > > > > /* > > * If the hole extends beyond i_size, set the hole > > @@ -4789,18 +4791,6 @@ int ext4_ext_punch_hole(struct file *file, loff_t offset, loff_t length) > > first_page_offset = first_page << PAGE_CACHE_SHIFT; > > last_page_offset = last_page << PAGE_CACHE_SHIFT; > > > > - /* > > - * Write out all dirty pages to avoid race conditions > > - * Then release them. > > - */ > > - if (mapping->nrpages && mapping_tagged(mapping, PAGECACHE_TAG_DIRTY)) { > > - err = filemap_write_and_wait_range(mapping, > > - offset, offset + length - 1); > > - > > - if (err) > > - return err; > > - } > > - > > Removing above code will cause a problem in case the file has all its > data in memory and nothing has been committed on disk. If punch hole > is issued for such a file, as there are no extents present, EIO would > be returned from ext4_ext_rm_leaf. So, even though blocks would be > removed from memory, the end result will be error EIO. > > > /* Now release the pages */ > > if (last_page_offset > first_page_offset) { > > truncate_pagecache_range(inode, first_page_offset, > > To avoid this, you can add a check after the call to truncate_pagecache_range. > if(!inode->i_blocks) > return 0; Thanks for pointing this out. However Dimitry has better fix for this with some additional changes so I am dropping this particular patch. (see "ext4: punch_hole should wait for DIO writers") Thanks! -Lukas > > > @@ -4812,12 +4802,14 @@ int ext4_ext_punch_hole(struct file *file, loff_t offset, loff_t length) > > > > credits = ext4_writepage_trans_blocks(inode); > > handle = ext4_journal_start(inode, credits); > > - if (IS_ERR(handle)) > > - return PTR_ERR(handle); > > + if (IS_ERR(handle)) { > > + err = PTR_ERR(handle); > > + goto out1; > > + } > > > > err = ext4_orphan_add(handle, inode); > > if (err) > > - goto out; > > + goto out1; > > > > /* > > * Now we need to zero out the non-page-aligned data in the > > @@ -4907,6 +4899,8 @@ out: > > inode->i_mtime = inode->i_ctime = ext4_current_time(inode); > > ext4_mark_inode_dirty(handle, inode); > > ext4_journal_stop(handle); > > +out1: > > + mutex_unlock(&inode->i_mutex); > > return err; > > } > > int ext4_fiemap(struct inode *inode, struct fiemap_extent_info *fieinfo, > > -- > > 1.7.7.6 > > > > -- > > To unsubscribe from this list: send the line "unsubscribe linux-ext4" in > > the body of a message to majordomo@vger.kernel.org > > More majordomo info at http://vger.kernel.org/majordomo-info.html > -- To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to majordomo@kvack.org. For more info on Linux MM, see: http://www.linux-mm.org/ . Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
next prev parent reply other threads:[~2012-09-13 15:15 UTC|newest] Thread overview: 32+ messages / expand[flat|nested] mbox.gz Atom feed top 2012-08-31 22:21 [PATCH 00/15 v2] Add invalidatepage_range address space operation Lukas Czerner 2012-08-31 22:21 ` [PATCH 01/15 v2] mm: add " Lukas Czerner 2012-09-04 23:43 ` Andrew Morton 2012-09-05 14:36 ` Lukáš Czerner 2012-09-05 15:56 ` Christoph Hellwig 2012-09-05 15:56 ` Christoph Hellwig 2012-09-05 16:42 ` Lukáš Czerner 2012-09-14 13:21 ` Lukáš Czerner 2012-09-14 13:21 ` Lukáš Czerner 2012-08-31 22:21 ` [PATCH 02/15 v2] jbd2: implement jbd2_journal_invalidatepage_range Lukas Czerner 2012-09-04 14:52 ` J. Bruce Fields 2012-09-04 14:52 ` J. Bruce Fields 2012-09-04 15:37 ` Lukáš Czerner 2012-09-04 17:44 ` J. Bruce Fields 2012-09-04 17:44 ` J. Bruce Fields 2012-08-31 22:21 ` [PATCH 03/15 v2] ext4: implement invalidatepage_range aop Lukas Czerner 2012-08-31 22:21 ` [PATCH 04/15 v2] xfs: " Lukas Czerner 2012-08-31 22:21 ` Lukas Czerner 2012-08-31 22:21 ` [PATCH 05/15 v2] ocfs2: " Lukas Czerner 2012-08-31 22:21 ` [PATCH 06/15 v2] mm: teach truncate_inode_pages_range() to handle non page aligned ranges Lukas Czerner 2012-08-31 22:21 ` [PATCH 07/15 v2] ext4: Take i_mutex before punching hole Lukas Czerner 2012-09-10 12:00 ` Ashish Sangwan 2012-09-13 15:15 ` Lukáš Czerner [this message] 2012-09-13 15:15 ` Lukáš Czerner 2012-08-31 22:21 ` [PATCH 08/15 v2] Revert "ext4: remove no longer used functions in inode.c" Lukas Czerner 2012-08-31 22:21 ` [PATCH 09/15 v2] Revert "ext4: fix fsx truncate failure" Lukas Czerner 2012-08-31 22:21 ` [PATCH 10/15 v2] ext4: use ext4_zero_partial_blocks in punch_hole Lukas Czerner 2012-08-31 22:21 ` [PATCH 11/15 v2] ext4: remove unused discard_partial_page_buffers Lukas Czerner 2012-08-31 22:21 ` [PATCH 12/15 v2] ext4: remove unused code from ext4_remove_blocks() Lukas Czerner 2012-08-31 22:21 ` [PATCH 13/15 v2] ext4: update ext4_ext_remove_space trace point Lukas Czerner 2012-08-31 22:21 ` [PATCH 14/15 v2] ext4: make punch hole code path work with bigalloc Lukas Czerner 2012-08-31 22:21 ` [PATCH 15/15 v2] ext4: Allow punch hole with bigalloc enabled Lukas Czerner
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=alpine.LFD.2.00.1209131113450.15781@dhcp-196-88.bos.redhat.com \ --to=lczerner@redhat.com \ --cc=ashishsangwan2@gmail.com \ --cc=hughd@google.com \ --cc=linux-ext4@vger.kernel.org \ --cc=linux-fsdevel@vger.kernel.org \ --cc=linux-mm@kvack.org \ --cc=tytso@mit.edu \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.