| Message ID | 1342185555-21146-2-git-send-email-lczerner@redhat.com |
|---|---|
| State | Superseded, archived |
| Headers | show |
Hi Lukas, This patch set has changes to the VFS, XFS, and ext4, and there are cross dependencies between them. Is there any way you can disentagle the dependencies between the patches so we don't need to worry about how these get pulled in during the merge window? I suppose could try to get the XFS folks to sign off with my carrying this patch in the ext4 tree, since the bulk of the changes are ext4-related, but it is a bit of a complication. - Ted -- 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
On Fri, Jul 13, 2012 at 01:42:09PM -0400, Theodore Ts'o wrote: > Hi Lukas, > > This patch set has changes to the VFS, XFS, and ext4, and there are > cross dependencies between them. Is there any way you can disentagle > the dependencies between the patches so we don't need to worry about > how these get pulled in during the merge window? > > I suppose could try to get the XFS folks to sign off with my carrying > this patch in the ext4 tree, since the bulk of the changes are > ext4-related, but it is a bit of a complication. There are no real XFS changes, just changing the calling conventions for a generic function. Adding this through another tree if properly reviewed seems perfectly fine. -- 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
On Fri, 13 Jul 2012, Theodore Ts'o wrote: > Date: Fri, 13 Jul 2012 13:42:09 -0400 > From: Theodore Ts'o <tytso@mit.edu> > To: Lukas Czerner <lczerner@redhat.com> > Cc: linux-ext4@vger.kernel.org, linux-fsdevel@vger.kernel.org, > achender@linux.vnet.ibm.com > Subject: Re: [PATCH 02/12 v2] Revert "ext4: fix fsx truncate failure" > > Hi Lukas, > > This patch set has changes to the VFS, XFS, and ext4, and there are > cross dependencies between them. Is there any way you can disentagle > the dependencies between the patches so we don't need to worry about > how these get pulled in during the merge window? > > I suppose could try to get the XFS folks to sign off with my carrying > this patch in the ext4 tree, since the bulk of the changes are > ext4-related, but it is a bit of a complication. > > - Ted Hi Ted, there is no VFS change, MM is probably what you've had in mind ? So the only reason why there are xfs, mm and tmpfs changes is that I am changing truncate_partial_page() and a small side effect is changed calling conventions. We can push patches 3, 4, 5 and 6 through the mm tree. But we'll have to make sure that it will land before ext4 changes since I am using the new truncate_partial_page() behaviour in ext4. Will that be possible ? Hugh ? -Lukas -- 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
On Mon, 16 Jul 2012, Lukas Czerner wrote: > On Fri, 13 Jul 2012, Theodore Ts'o wrote: > > Date: Fri, 13 Jul 2012 13:42:09 -0400 > > From: Theodore Ts'o <tytso@mit.edu> > > To: Lukas Czerner <lczerner@redhat.com> > > Cc: linux-ext4@vger.kernel.org, linux-fsdevel@vger.kernel.org, > > achender@linux.vnet.ibm.com > > Subject: Re: [PATCH 02/12 v2] Revert "ext4: fix fsx truncate failure" > > > > Hi Lukas, > > > > This patch set has changes to the VFS, XFS, and ext4, and there are > > cross dependencies between them. Is there any way you can disentagle > > the dependencies between the patches so we don't need to worry about > > how these get pulled in during the merge window? > > > > I suppose could try to get the XFS folks to sign off with my carrying > > this patch in the ext4 tree, since the bulk of the changes are > > ext4-related, but it is a bit of a complication. > > > > - Ted > > Hi Ted, > > there is no VFS change, MM is probably what you've had in mind ? So > the only reason why there are xfs, mm and tmpfs changes is that I am > changing truncate_partial_page() and a small side effect is changed > calling conventions. truncate_partial_page() is static inline to mm/truncate.c: was that a typo, or are you changing that too and I missed it? Sorry, but I find your changes of the infinity convention from -1 to LLONG_MAX just gratuitous and confusing (and error prone if we ever have to backport portions to earlier stable releases). It grieves me enough that unmap_mapping_range() has always had quite a different convention from truncate_inode_pages_range(). You're now proposing to give truncate_inode_pages_range() yet another convention different from invalidate_inode_pages2_range() and truncate_pagecache_range()? At cost of having to make xfs and tmpfs (well, actually it's the tiny !SHMEM ramfs case you're changing there in 03/12) dependent on synchronizing with your other changes? I can see that when I converted shmem_truncate_range() (later extended to shmem_undo_range()) to partial_start and partial_end, I did put in an ugly couple of lines if (lend == -1) end = -1; /* unsigned, so actually very big */ but I think it's better there than "lend == -1 ? LLONG_MAX : lend;" ugliness spread around in the filesystems. I don't see any upside: please, could you just drop those changes, so that then it comes down to synchronizing ext4 with mm/truncate.c? > > We can push patches 3, 4, 5 and 6 through the mm tree. But we'll > have to make sure that it will land before ext4 changes since I am > using the new truncate_partial_page() behaviour in ext4. Will that > be possible ? > > Hugh ? I'd like patchs 3, 4 and 5 to vanish. As to 6 (perhaps it won't be 6 after that!), I want to work through that one myself (I'll try this evening): it looked over-complicated to me, and I'd rather go back to what I worked out for partial_end in shmem_truncate_range(). Then yes, if we're all happy with the result of that, it will be best for Ted to take even the mm/truncate.c patch into his ext4 branch, made visible in linux-next before the merge window. We're running out of time: I'll make every effort to get back to you on 6 after I've tested late today. Hugh -- 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
On Mon, 16 Jul 2012, Hugh Dickins wrote: > Date: Mon, 16 Jul 2012 14:41:41 -0700 (PDT) > From: Hugh Dickins <hughd@google.com> > To: Lukas Czerner <lczerner@redhat.com> > Cc: Andrew Morton <akpm@linux-foundation.org>, Theodore Ts'o <tytso@mit.edu>, > Dave Chinner <dchinner@redhat.com>, linux-ext4@vger.kernel.org, > linux-fsdevel@vger.kernel.org, achender@linux.vnet.ibm.com > Subject: Re: [PATCH 02/12 v2] Revert "ext4: fix fsx truncate failure" > > On Mon, 16 Jul 2012, Lukas Czerner wrote: > > On Fri, 13 Jul 2012, Theodore Ts'o wrote: > > > Date: Fri, 13 Jul 2012 13:42:09 -0400 > > > From: Theodore Ts'o <tytso@mit.edu> > > > To: Lukas Czerner <lczerner@redhat.com> > > > Cc: linux-ext4@vger.kernel.org, linux-fsdevel@vger.kernel.org, > > > achender@linux.vnet.ibm.com > > > Subject: Re: [PATCH 02/12 v2] Revert "ext4: fix fsx truncate failure" > > > > > > Hi Lukas, > > > > > > This patch set has changes to the VFS, XFS, and ext4, and there are > > > cross dependencies between them. Is there any way you can disentagle > > > the dependencies between the patches so we don't need to worry about > > > how these get pulled in during the merge window? > > > > > > I suppose could try to get the XFS folks to sign off with my carrying > > > this patch in the ext4 tree, since the bulk of the changes are > > > ext4-related, but it is a bit of a complication. > > > > > > - Ted > > > > Hi Ted, > > > > there is no VFS change, MM is probably what you've had in mind ? So > > the only reason why there are xfs, mm and tmpfs changes is that I am > > changing truncate_partial_page() and a small side effect is changed > > calling conventions. > > truncate_partial_page() is static inline to mm/truncate.c: > was that a typo, or are you changing that too and I missed it? Sorry, I meant truncate_inode_pages_range(). > > Sorry, but I find your changes of the infinity convention from -1 to > LLONG_MAX just gratuitous and confusing (and error prone if we ever > have to backport portions to earlier stable releases). > > It grieves me enough that unmap_mapping_range() has always had quite > a different convention from truncate_inode_pages_range(). You're now > proposing to give truncate_inode_pages_range() yet another convention > different from invalidate_inode_pages2_range() and > truncate_pagecache_range()? > > At cost of having to make xfs and tmpfs (well, actually it's the > tiny !SHMEM ramfs case you're changing there in 03/12) dependent > on synchronizing with your other changes? > > I can see that when I converted shmem_truncate_range() (later extended > to shmem_undo_range()) to partial_start and partial_end, I did put in > an ugly couple of lines > if (lend == -1) > end = -1; /* unsigned, so actually very big */ > but I think it's better there than "lend == -1 ? LLONG_MAX : lend;" > ugliness spread around in the filesystems. I find the -1 convention rather confusing and the above, as you said, quite ugly. I seemed to me much cleaner to just send what we actually want, which is LLONG_MAX. You're right that "lend == -1 ? LLONG_MAX : lend;" is not pretty, but it at least gives the file systems reason to fix that and do not use -1. It is bad enough that zero_user_segment() and friends has absolutely weird convention where instead of "end" you actually expect "end + 1", whereas zero_user() actually uses start + size instead. Things like that, plus the -1, makes it easy to confuse (at least for me). > > I don't see any upside: please, could you just drop those changes, > so that then it comes down to synchronizing ext4 with mm/truncate.c? The reason of this change to teach truncate_inode_pages_range() to handle non page aligned ranges which we then can make use of in ext4. > > > > > We can push patches 3, 4, 5 and 6 through the mm tree. But we'll > > have to make sure that it will land before ext4 changes since I am > > using the new truncate_partial_page() behaviour in ext4. Will that > > be possible ? > > > > Hugh ? > > I'd like patchs 3, 4 and 5 to vanish. As to 6 (perhaps it won't be > 6 after that!), I want to work through that one myself (I'll try this > evening): it looked over-complicated to me, and I'd rather go back to > what I worked out for partial_end in shmem_truncate_range(). The code in the shmem_undo_range() is really similar, except it is not able to handle offset of LLONG_MAX, or actually even the last page with this offset, but truncate_inode_pages_range() obviously does. But again, there is the confusion with the "end" being "end + 1" which seems odd. > > Then yes, if we're all happy with the result of that, it will be best > for Ted to take even the mm/truncate.c patch into his ext4 branch, > made visible in linux-next before the merge window. > > We're running out of time: I'll make every effort to get back to you > on 6 after I've tested late today. Great, thanks Hugh. -Lukas > > Hugh > -- 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
On 7/13/12 8:19 AM, Lukas Czerner wrote: > This reverts commit 189e868fa8fdca702eb9db9d8afc46b5cb9144c9. > > This commit reintroduces the use of ext4_block_truncate_page() in ext4 > truncate operation instead of ext4_discard_partial_page_buffers(). And I think commit 15291164b22a357cb211b618adfef4fa82fc0de3 jbd2: clear BH_Delay & BH_Unwritten in journal_unmap_buffer fixed the fsx problem referenced in that commit more properly. > The statement in the commit description that the truncate operation only > zero block unaligned portion of the last page is not exactly right, > since truncate_pagecache_range() also zeroes and invalidate the unaligned > portion of the page. Then there is no need to zero and unmap it once more > and ext4_block_truncate_page() was doing the right job, although we > still need to update the buffer head containing the last block, which is > exactly what ext4_block_truncate_page() is doing. > > This was tested on ppc64 machine with block size of 1024 bytes without > any problems. > > Signed-off-by: Lukas Czerner <lczerner@redhat.com> > --- > fs/ext4/extents.c | 13 ++----------- > fs/ext4/indirect.c | 13 ++----------- > 2 files changed, 4 insertions(+), 22 deletions(-) > > diff --git a/fs/ext4/extents.c b/fs/ext4/extents.c > index 91341ec..ceab5f5 100644 > --- a/fs/ext4/extents.c > +++ b/fs/ext4/extents.c > @@ -4222,7 +4222,6 @@ void ext4_ext_truncate(struct inode *inode) > struct super_block *sb = inode->i_sb; > ext4_lblk_t last_block; > handle_t *handle; > - loff_t page_len; > int err = 0; > > /* > @@ -4239,16 +4238,8 @@ void ext4_ext_truncate(struct inode *inode) > if (IS_ERR(handle)) > return; > > - if (inode->i_size % PAGE_CACHE_SIZE != 0) { > - page_len = PAGE_CACHE_SIZE - > - (inode->i_size & (PAGE_CACHE_SIZE - 1)); > - > - err = ext4_discard_partial_page_buffers(handle, > - mapping, inode->i_size, page_len, 0); > - > - if (err) > - goto out_stop; > - } > + if (inode->i_size & (sb->s_blocksize - 1)) > + ext4_block_truncate_page(handle, mapping, inode->i_size); > > if (ext4_orphan_add(handle, inode)) > goto out_stop; > diff --git a/fs/ext4/indirect.c b/fs/ext4/indirect.c > index 830e1b2..a082b30 100644 > --- a/fs/ext4/indirect.c > +++ b/fs/ext4/indirect.c > @@ -1349,9 +1349,7 @@ void ext4_ind_truncate(struct inode *inode) > __le32 nr = 0; > int n = 0; > ext4_lblk_t last_block, max_block; > - loff_t page_len; > unsigned blocksize = inode->i_sb->s_blocksize; > - int err; > > handle = start_transaction(inode); > if (IS_ERR(handle)) > @@ -1362,16 +1360,9 @@ void ext4_ind_truncate(struct inode *inode) > max_block = (EXT4_SB(inode->i_sb)->s_bitmap_maxbytes + blocksize-1) > >> EXT4_BLOCK_SIZE_BITS(inode->i_sb); > > - if (inode->i_size % PAGE_CACHE_SIZE != 0) { > - page_len = PAGE_CACHE_SIZE - > - (inode->i_size & (PAGE_CACHE_SIZE - 1)); > - > - err = ext4_discard_partial_page_buffers(handle, > - mapping, inode->i_size, page_len, 0); > - > - if (err) > + if (inode->i_size & (blocksize - 1)) > + if (ext4_block_truncate_page(handle, mapping, inode->i_size)) > goto out_stop; > - } > > if (last_block != max_block) { > n = ext4_block_to_path(inode, last_block, offsets, NULL); > -- 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
On Wed, 18 Jul 2012, Eric Sandeen wrote: > Date: Wed, 18 Jul 2012 14:34:47 -0500 > From: Eric Sandeen <sandeen@redhat.com> > To: Lukas Czerner <lczerner@redhat.com> > Cc: linux-ext4@vger.kernel.org, linux-fsdevel@vger.kernel.org, tytso@mit.edu, > achender@linux.vnet.ibm.com > Subject: Re: [PATCH 02/12 v2] Revert "ext4: fix fsx truncate failure" > > On 7/13/12 8:19 AM, Lukas Czerner wrote: > > This reverts commit 189e868fa8fdca702eb9db9d8afc46b5cb9144c9. > > > > This commit reintroduces the use of ext4_block_truncate_page() in ext4 > > truncate operation instead of ext4_discard_partial_page_buffers(). > > And I think commit 15291164b22a357cb211b618adfef4fa82fc0de3 > jbd2: clear BH_Delay & BH_Unwritten in journal_unmap_buffer > > fixed the fsx problem referenced in that commit more properly. You're probably right, I'll test that with your test case and add it to the description. Thanks! -Lukas > > > The statement in the commit description that the truncate operation only > > zero block unaligned portion of the last page is not exactly right, > > since truncate_pagecache_range() also zeroes and invalidate the unaligned > > portion of the page. Then there is no need to zero and unmap it once more > > and ext4_block_truncate_page() was doing the right job, although we > > still need to update the buffer head containing the last block, which is > > exactly what ext4_block_truncate_page() is doing. > > > > This was tested on ppc64 machine with block size of 1024 bytes without > > any problems. > > > > Signed-off-by: Lukas Czerner <lczerner@redhat.com> > > --- > > fs/ext4/extents.c | 13 ++----------- > > fs/ext4/indirect.c | 13 ++----------- > > 2 files changed, 4 insertions(+), 22 deletions(-) > > > > diff --git a/fs/ext4/extents.c b/fs/ext4/extents.c > > index 91341ec..ceab5f5 100644 > > --- a/fs/ext4/extents.c > > +++ b/fs/ext4/extents.c > > @@ -4222,7 +4222,6 @@ void ext4_ext_truncate(struct inode *inode) > > struct super_block *sb = inode->i_sb; > > ext4_lblk_t last_block; > > handle_t *handle; > > - loff_t page_len; > > int err = 0; > > > > /* > > @@ -4239,16 +4238,8 @@ void ext4_ext_truncate(struct inode *inode) > > if (IS_ERR(handle)) > > return; > > > > - if (inode->i_size % PAGE_CACHE_SIZE != 0) { > > - page_len = PAGE_CACHE_SIZE - > > - (inode->i_size & (PAGE_CACHE_SIZE - 1)); > > - > > - err = ext4_discard_partial_page_buffers(handle, > > - mapping, inode->i_size, page_len, 0); > > - > > - if (err) > > - goto out_stop; > > - } > > + if (inode->i_size & (sb->s_blocksize - 1)) > > + ext4_block_truncate_page(handle, mapping, inode->i_size); > > > > if (ext4_orphan_add(handle, inode)) > > goto out_stop; > > diff --git a/fs/ext4/indirect.c b/fs/ext4/indirect.c > > index 830e1b2..a082b30 100644 > > --- a/fs/ext4/indirect.c > > +++ b/fs/ext4/indirect.c > > @@ -1349,9 +1349,7 @@ void ext4_ind_truncate(struct inode *inode) > > __le32 nr = 0; > > int n = 0; > > ext4_lblk_t last_block, max_block; > > - loff_t page_len; > > unsigned blocksize = inode->i_sb->s_blocksize; > > - int err; > > > > handle = start_transaction(inode); > > if (IS_ERR(handle)) > > @@ -1362,16 +1360,9 @@ void ext4_ind_truncate(struct inode *inode) > > max_block = (EXT4_SB(inode->i_sb)->s_bitmap_maxbytes + blocksize-1) > > >> EXT4_BLOCK_SIZE_BITS(inode->i_sb); > > > > - if (inode->i_size % PAGE_CACHE_SIZE != 0) { > > - page_len = PAGE_CACHE_SIZE - > > - (inode->i_size & (PAGE_CACHE_SIZE - 1)); > > - > > - err = ext4_discard_partial_page_buffers(handle, > > - mapping, inode->i_size, page_len, 0); > > - > > - if (err) > > + if (inode->i_size & (blocksize - 1)) > > + if (ext4_block_truncate_page(handle, mapping, inode->i_size)) > > goto out_stop; > > - } > > > > if (last_block != max_block) { > > n = ext4_block_to_path(inode, last_block, offsets, NULL); > > > > > -- 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
diff --git a/fs/ext4/extents.c b/fs/ext4/extents.c index 91341ec..ceab5f5 100644 --- a/fs/ext4/extents.c +++ b/fs/ext4/extents.c @@ -4222,7 +4222,6 @@ void ext4_ext_truncate(struct inode *inode) struct super_block *sb = inode->i_sb; ext4_lblk_t last_block; handle_t *handle; - loff_t page_len; int err = 0; /* @@ -4239,16 +4238,8 @@ void ext4_ext_truncate(struct inode *inode) if (IS_ERR(handle)) return; - if (inode->i_size % PAGE_CACHE_SIZE != 0) { - page_len = PAGE_CACHE_SIZE - - (inode->i_size & (PAGE_CACHE_SIZE - 1)); - - err = ext4_discard_partial_page_buffers(handle, - mapping, inode->i_size, page_len, 0); - - if (err) - goto out_stop; - } + if (inode->i_size & (sb->s_blocksize - 1)) + ext4_block_truncate_page(handle, mapping, inode->i_size); if (ext4_orphan_add(handle, inode)) goto out_stop; diff --git a/fs/ext4/indirect.c b/fs/ext4/indirect.c index 830e1b2..a082b30 100644 --- a/fs/ext4/indirect.c +++ b/fs/ext4/indirect.c @@ -1349,9 +1349,7 @@ void ext4_ind_truncate(struct inode *inode) __le32 nr = 0; int n = 0; ext4_lblk_t last_block, max_block; - loff_t page_len; unsigned blocksize = inode->i_sb->s_blocksize; - int err; handle = start_transaction(inode); if (IS_ERR(handle)) @@ -1362,16 +1360,9 @@ void ext4_ind_truncate(struct inode *inode) max_block = (EXT4_SB(inode->i_sb)->s_bitmap_maxbytes + blocksize-1) >> EXT4_BLOCK_SIZE_BITS(inode->i_sb); - if (inode->i_size % PAGE_CACHE_SIZE != 0) { - page_len = PAGE_CACHE_SIZE - - (inode->i_size & (PAGE_CACHE_SIZE - 1)); - - err = ext4_discard_partial_page_buffers(handle, - mapping, inode->i_size, page_len, 0); - - if (err) + if (inode->i_size & (blocksize - 1)) + if (ext4_block_truncate_page(handle, mapping, inode->i_size)) goto out_stop; - } if (last_block != max_block) { n = ext4_block_to_path(inode, last_block, offsets, NULL);
This reverts commit 189e868fa8fdca702eb9db9d8afc46b5cb9144c9. This commit reintroduces the use of ext4_block_truncate_page() in ext4 truncate operation instead of ext4_discard_partial_page_buffers(). The statement in the commit description that the truncate operation only zero block unaligned portion of the last page is not exactly right, since truncate_pagecache_range() also zeroes and invalidate the unaligned portion of the page. Then there is no need to zero and unmap it once more and ext4_block_truncate_page() was doing the right job, although we still need to update the buffer head containing the last block, which is exactly what ext4_block_truncate_page() is doing. This was tested on ppc64 machine with block size of 1024 bytes without any problems. Signed-off-by: Lukas Czerner <lczerner@redhat.com> --- fs/ext4/extents.c | 13 ++----------- fs/ext4/indirect.c | 13 ++----------- 2 files changed, 4 insertions(+), 22 deletions(-)