Patchwork [RFC,3/8,v2] ext4: initialize extent status tree

login
register
mail settings
Submitter Zheng Liu
Date Aug. 22, 2012, 6:05 a.m.
Message ID <1345615545-26133-4-git-send-email-wenqing.lz@taobao.com>
Download mbox | patch
Permalink /patch/179228/
State Accepted, archived
Headers show

Comments

Zheng Liu - Aug. 22, 2012, 6:05 a.m.
From: Zheng Liu <wenqing.lz@taobao.com>

Let ext4 initialize extent status tree of an inode.

Signed-off-by: Yongqiang Yang <xiaoqiangnk@gmail.com>
Signed-off-by: Allison Henderson <achender@linux.vnet.ibm.com>
Signed-off-by: Zheng Liu <wenqing.lz@taobao.com>
---
 fs/ext4/super.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)
Theodore Ts'o - Sept. 19, 2012, 6:53 p.m.
On Wed, Aug 22, 2012 at 02:05:40PM +0800, Zheng Liu wrote:
> From: Zheng Liu <wenqing.lz@taobao.com>
> 
> Let ext4 initialize extent status tree of an inode.
> 
> Signed-off-by: Yongqiang Yang <xiaoqiangnk@gmail.com>
> Signed-off-by: Allison Henderson <achender@linux.vnet.ibm.com>
> Signed-off-by: Zheng Liu <wenqing.lz@taobao.com>

One general comment --- "Signed-off-by" should only be used when
someone actually handles a patch; that is, you add the Signed-off-by
for yourself if you originally wrote the patch, or if you started with
someone else's patch and modified it.

If you merely reviewed the patch, you can request that the patch
author add a "Reviewed-by: " header.

The other footer that you will sometimes see is "Acked-by: ".  This
gets used by subsystem maintainers who approve of a patch, but instead
of their handling the patch, they are going to let the patch flow into
the upstream via some other tree.

So for example, Lukas's invalidate_page_range patch set will require
an "Acked-by: " footer from the XFS, OCFS2, and mm maintainers for
those commits which touch their subsystems.  Once we've gotten an OK
from those folks, then when I accept those patches into the ext4 tree,
I will add my "Signed-off-by: " header since the patches flowed
through my tree.

These are minor points, but Linus will sometimes complain if these
footers aren't used correctly, since Signed-off-by: does have a very
specific legal meaning.  See: http://elinux.org/Developer_Certificate_Of_Origin

In any case, I've changed the Signed-off-by footers for Yongqiang Yang
and Allison Heanderson to be "Reviewed-by: ", in the commit
descriptions, since I believe that should be the more appropriate
footer to be using in this case.  (If you did actually use some code
from Yongqiang or Allison from patches which they had previously sent
with a DCO, then Signed-off-by would be correct.  Let me know if
that's the case....)

Regards,

						- 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
Theodore Ts'o - Sept. 19, 2012, 7:05 p.m.
On Wed, Aug 22, 2012 at 02:05:40PM +0800, Zheng Liu wrote:
> diff --git a/fs/ext4/super.c b/fs/ext4/super.c
> index 3e0851e..353b1fd 100644
> --- a/fs/ext4/super.c
> +++ b/fs/ext4/super.c
> @@ -944,6 +944,7 @@ static struct inode *ext4_alloc_inode(struct super_block *sb)
>  	memset(&ei->i_cached_extent, 0, sizeof(struct ext4_ext_cache));
>  	INIT_LIST_HEAD(&ei->i_prealloc_list);
>  	spin_lock_init(&ei->i_prealloc_lock);
> +	ext4_es_init_tree(&ei->i_es_tree);
>  	ei->i_reserved_data_blocks = 0;
>  	ei->i_reserved_meta_blocks = 0;
>  	ei->i_allocated_meta_blocks = 0;

This patch hunk immediately me ask, "so when does the extent_status
tree get freed?"  And I believe the answer is that currently, since it
only tracks delayed extents (and we're not using it for locking
purposes), by the time we have evicted the inode and are ready to call
ext4_clear_inode(), we should have released all of the nodes in the
ext4_es_tree.   Is that correct?

If so, we might want to think about adding a sanity check to make sure
that by the time we are done with the inode in ext4_evict_inode()
(after we have forced writeback), the ext4_es_tree is empty.  Agreed?

       	       	      		      		   - 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
Zheng Liu - Sept. 24, 2012, 4:45 a.m.
On Wed, Sep 19, 2012 at 03:05:41PM -0400, Theodore Ts'o wrote:
> On Wed, Aug 22, 2012 at 02:05:40PM +0800, Zheng Liu wrote:
> > diff --git a/fs/ext4/super.c b/fs/ext4/super.c
> > index 3e0851e..353b1fd 100644
> > --- a/fs/ext4/super.c
> > +++ b/fs/ext4/super.c
> > @@ -944,6 +944,7 @@ static struct inode *ext4_alloc_inode(struct super_block *sb)
> >  	memset(&ei->i_cached_extent, 0, sizeof(struct ext4_ext_cache));
> >  	INIT_LIST_HEAD(&ei->i_prealloc_list);
> >  	spin_lock_init(&ei->i_prealloc_lock);
> > +	ext4_es_init_tree(&ei->i_es_tree);
> >  	ei->i_reserved_data_blocks = 0;
> >  	ei->i_reserved_meta_blocks = 0;
> >  	ei->i_allocated_meta_blocks = 0;
> 
> This patch hunk immediately me ask, "so when does the extent_status
> tree get freed?"  And I believe the answer is that currently, since it
> only tracks delayed extents (and we're not using it for locking
> purposes), by the time we have evicted the inode and are ready to call
> ext4_clear_inode(), we should have released all of the nodes in the
> ext4_es_tree.   Is that correct?
> 
> If so, we might want to think about adding a sanity check to make sure
> that by the time we are done with the inode in ext4_evict_inode()
> (after we have forced writeback), the ext4_es_tree is empty.  Agreed?

Yes, I agree with you.  I will add a sanity check. :-)

Regards,
Zheng
--
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
Zheng Liu - Sept. 25, 2012, 12:42 p.m.
On Wed, Sep 19, 2012 at 03:05:41PM -0400, Theodore Ts'o wrote:
> On Wed, Aug 22, 2012 at 02:05:40PM +0800, Zheng Liu wrote:
> > diff --git a/fs/ext4/super.c b/fs/ext4/super.c
> > index 3e0851e..353b1fd 100644
> > --- a/fs/ext4/super.c
> > +++ b/fs/ext4/super.c
> > @@ -944,6 +944,7 @@ static struct inode *ext4_alloc_inode(struct super_block *sb)
> >  	memset(&ei->i_cached_extent, 0, sizeof(struct ext4_ext_cache));
> >  	INIT_LIST_HEAD(&ei->i_prealloc_list);
> >  	spin_lock_init(&ei->i_prealloc_lock);
> > +	ext4_es_init_tree(&ei->i_es_tree);
> >  	ei->i_reserved_data_blocks = 0;
> >  	ei->i_reserved_meta_blocks = 0;
> >  	ei->i_allocated_meta_blocks = 0;
> 
> This patch hunk immediately me ask, "so when does the extent_status
> tree get freed?"  And I believe the answer is that currently, since it
> only tracks delayed extents (and we're not using it for locking
> purposes), by the time we have evicted the inode and are ready to call
> ext4_clear_inode(), we should have released all of the nodes in the
> ext4_es_tree.   Is that correct?
> 
> If so, we might want to think about adding a sanity check to make sure
> that by the time we are done with the inode in ext4_evict_inode()
> (after we have forced writeback), the ext4_es_tree is empty.  Agreed?

Hi Ted,

Today I revise this patch again, and I find extent_status_tree is freed
in ext4_clear_inode().  So maybe I don't think that we need to check
this tree to be freed in ext4_evict_inode().  This change is in this
patch '[RFC][PATCH 4/8 v2] ext4: let ext4 maintain extent status tree'.
What's your opinion?

Regards,
Zheng
--
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
Theodore Ts'o - Sept. 25, 2012, 8:59 p.m.
On Tue, Sep 25, 2012 at 08:42:52PM +0800, Zheng Liu wrote:
> > If so, we might want to think about adding a sanity check to make sure
> > that by the time we are done with the inode in ext4_evict_inode()
> > (after we have forced writeback), the ext4_es_tree is empty.  Agreed?
> 
> Today I revise this patch again, and I find extent_status_tree is freed
> in ext4_clear_inode().  So maybe I don't think that we need to check
> this tree to be freed in ext4_evict_inode().  This change is in this
> patch '[RFC][PATCH 4/8 v2] ext4: let ext4 maintain extent status tree'.
> What's your opinion?

When you say "revise this patch again", does that mean that you would
like to submit a new set of patch series with changes?  Or just that
you are looking at this patch set again?

It's certainly true that ext4_evict_inode() will call
ext4_clear_inode(), so it's not a question of worrying about a memory
leak.  I was thinking more about doing this as a cheap sanity check
for the data structure.  By the time we call ext4_evict_inode(), the
mm layer all writeback should be complete.  Hence, all of the entries
to the tree _should_ have been removed by the time we call
ext4_evict_inode().

I don't know if this is going to change as you start using this data
structure for other purposes (such as locking a range of pages), but
if I understand how things are currently working, it _should_ be the
case that when ext4_evict_inode() calls ext4_clear_inode(), the call
to ext4_es_remove_extent() should be a no-op, since all of the nodes
in the extent status tree should have been released by then.  If it
isn't, then either I'm not understanding the code, or there's a bug in
the code.

						- 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
Zheng Liu - Sept. 26, 2012, 2:09 a.m.
On Tue, Sep 25, 2012 at 04:59:21PM -0400, Theodore Ts'o wrote:
> On Tue, Sep 25, 2012 at 08:42:52PM +0800, Zheng Liu wrote:
> > > If so, we might want to think about adding a sanity check to make sure
> > > that by the time we are done with the inode in ext4_evict_inode()
> > > (after we have forced writeback), the ext4_es_tree is empty.  Agreed?
> > 
> > Today I revise this patch again, and I find extent_status_tree is freed
> > in ext4_clear_inode().  So maybe I don't think that we need to check
> > this tree to be freed in ext4_evict_inode().  This change is in this
> > patch '[RFC][PATCH 4/8 v2] ext4: let ext4 maintain extent status tree'.
> > What's your opinion?
> 
> When you say "revise this patch again", does that mean that you would
> like to submit a new set of patch series with changes?  Or just that
> you are looking at this patch set again?

Yes, I prepare to submit a new patch set.

> 
> It's certainly true that ext4_evict_inode() will call
> ext4_clear_inode(), so it's not a question of worrying about a memory
> leak.  I was thinking more about doing this as a cheap sanity check
> for the data structure.  By the time we call ext4_evict_inode(), the
> mm layer all writeback should be complete.  Hence, all of the entries
> to the tree _should_ have been removed by the time we call
> ext4_evict_inode().
> 
> I don't know if this is going to change as you start using this data
> structure for other purposes (such as locking a range of pages), but
> if I understand how things are currently working, it _should_ be the
> case that when ext4_evict_inode() calls ext4_clear_inode(), the call
> to ext4_es_remove_extent() should be a no-op, since all of the nodes
> in the extent status tree should have been released by then.  If it
> isn't, then either I'm not understanding the code, or there's a bug in
> the code.

Yes, you are right.  In currently implementation, extent status tree
only maintains the status of delay extents.  So in ext4_evict_inode()
extent status tree should be an empty tree.  I will add a sanity check
to ensure it.  Thanks for your explanation.

Regards,
Zheng
--
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
Theodore Ts'o - Sept. 26, 2012, 2:47 a.m.
On Wed, Sep 26, 2012 at 10:09:55AM +0800, Zheng Liu wrote:
> On Tue, Sep 25, 2012 at 04:59:21PM -0400, Theodore Ts'o wrote:
> > On Tue, Sep 25, 2012 at 08:42:52PM +0800, Zheng Liu wrote:
> > > > If so, we might want to think about adding a sanity check to make sure
> > > > that by the time we are done with the inode in ext4_evict_inode()
> > > > (after we have forced writeback), the ext4_es_tree is empty.  Agreed?
> > > 
> > > Today I revise this patch again, and I find extent_status_tree is freed
> > > in ext4_clear_inode().  So maybe I don't think that we need to check
> > > this tree to be freed in ext4_evict_inode().  This change is in this
> > > patch '[RFC][PATCH 4/8 v2] ext4: let ext4 maintain extent status tree'.
> > > What's your opinion?
> > 
> > When you say "revise this patch again", does that mean that you would
> > like to submit a new set of patch series with changes?  Or just that
> > you are looking at this patch set again?
> 
> Yes, I prepare to submit a new patch set.

Well, note that the merge window is opening *soon*.  I haven't yet
moved the master branch, so I can update the patch set, but I'm going
to need it soon.

Can you let me know what changes you need to make?  If it is to add
new features or new sanity checks, does it make sense to simply make
it as new commits to existing patch set?  Or are there fundamental
problems with the current set, that would be better to fix in the
current set of commits?  (Or is it just minor stylistic/spelling
fixes?)

Thanks!!

					- 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
Zheng Liu - Sept. 26, 2012, 3:24 a.m.
On Tue, Sep 25, 2012 at 10:47:45PM -0400, Theodore Ts'o wrote:
> On Wed, Sep 26, 2012 at 10:09:55AM +0800, Zheng Liu wrote:
> > On Tue, Sep 25, 2012 at 04:59:21PM -0400, Theodore Ts'o wrote:
> > > On Tue, Sep 25, 2012 at 08:42:52PM +0800, Zheng Liu wrote:
> > > > > If so, we might want to think about adding a sanity check to make sure
> > > > > that by the time we are done with the inode in ext4_evict_inode()
> > > > > (after we have forced writeback), the ext4_es_tree is empty.  Agreed?
> > > > 
> > > > Today I revise this patch again, and I find extent_status_tree is freed
> > > > in ext4_clear_inode().  So maybe I don't think that we need to check
> > > > this tree to be freed in ext4_evict_inode().  This change is in this
> > > > patch '[RFC][PATCH 4/8 v2] ext4: let ext4 maintain extent status tree'.
> > > > What's your opinion?
> > > 
> > > When you say "revise this patch again", does that mean that you would
> > > like to submit a new set of patch series with changes?  Or just that
> > > you are looking at this patch set again?
> > 
> > Yes, I prepare to submit a new patch set.
> 
> Well, note that the merge window is opening *soon*.  I haven't yet
> moved the master branch, so I can update the patch set, but I'm going
> to need it soon.
> 
> Can you let me know what changes you need to make?  If it is to add
> new features or new sanity checks, does it make sense to simply make
> it as new commits to existing patch set?  Or are there fundamental
> problems with the current set, that would be better to fix in the
> current set of commits?  (Or is it just minor stylistic/spelling
> fixes?)
> 
> Thanks!!

In new patch set, there is three changes as beblow:

1. add a sanity check in ext4_evict_inode()
2. fix a bug in ext4_find_delalloc_range().  This bug is reported by
xfstest #230 when we enable bigalloc feature.
3. Add a new rwlock to protect extent status tree.

So I think that we can only add a sanity check and fix the bigalloc bug,
and then apply this patch set because the changes are minor.  For adding
a new lock to protect extent status tree, we can add this feature in a
new patch.  If you think it is OK, I can generate a new patch set, do
some tests using xfstest, and submit it as soon as possible.  What's
your opinion?

Regards,
Zheng
--
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
Theodore Ts'o - Sept. 26, 2012, 3:37 a.m.
On Wed, Sep 26, 2012 at 11:24:26AM +0800, Zheng Liu wrote:
> > Can you let me know what changes you need to make?  If it is to add
> > new features or new sanity checks, does it make sense to simply make
> > it as new commits to existing patch set?  Or are there fundamental
> > problems with the current set, that would be better to fix in the
> > current set of commits?  (Or is it just minor stylistic/spelling
> > fixes?)
> > 
> > Thanks!!
> 
> In new patch set, there is three changes as beblow:
> 
> 1. add a sanity check in ext4_evict_inode()
> 2. fix a bug in ext4_find_delalloc_range().  This bug is reported by
> xfstest #230 when we enable bigalloc feature.
> 3. Add a new rwlock to protect extent status tree.
> 
> So I think that we can only add a sanity check and fix the bigalloc bug,
> and then apply this patch set because the changes are minor.  For adding
> a new lock to protect extent status tree, we can add this feature in a
> new patch.  If you think it is OK, I can generate a new patch set, do
> some tests using xfstest, and submit it as soon as possible.  What's
> your opinion?

Do you think you can get me the patches by the end of the week?  If
so, that should work.

Thanks!!

						- 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
Theodore Ts'o - Sept. 26, 2012, 3:46 a.m.
On Wed, Sep 26, 2012 at 11:54:48AM +0800, Zheng Liu wrote:
> 
> Before this weekend, I can finish a new patch set with (1) adding a sanity
> check and (2) fixing bigalloc bug.  For adding a new lock, it is
> unstable now.  So this lock will not be included in this patch set.

That sounds good to me.  Adding a rw lock is something we can do in a
separate patch (which might not make the merge window for 3.7,
depending on when Linus decides to release 3.6, but that's OK).

					- 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
Zheng Liu - Sept. 26, 2012, 3:54 a.m.
On Tue, Sep 25, 2012 at 11:37:40PM -0400, Theodore Ts'o wrote:
> On Wed, Sep 26, 2012 at 11:24:26AM +0800, Zheng Liu wrote:
> > > Can you let me know what changes you need to make?  If it is to add
> > > new features or new sanity checks, does it make sense to simply make
> > > it as new commits to existing patch set?  Or are there fundamental
> > > problems with the current set, that would be better to fix in the
> > > current set of commits?  (Or is it just minor stylistic/spelling
> > > fixes?)
> > > 
> > > Thanks!!
> > 
> > In new patch set, there is three changes as beblow:
> > 
> > 1. add a sanity check in ext4_evict_inode()
> > 2. fix a bug in ext4_find_delalloc_range().  This bug is reported by
> > xfstest #230 when we enable bigalloc feature.
> > 3. Add a new rwlock to protect extent status tree.
> > 
> > So I think that we can only add a sanity check and fix the bigalloc bug,
> > and then apply this patch set because the changes are minor.  For adding
> > a new lock to protect extent status tree, we can add this feature in a
> > new patch.  If you think it is OK, I can generate a new patch set, do
> > some tests using xfstest, and submit it as soon as possible.  What's
> > your opinion?
> 
> Do you think you can get me the patches by the end of the week?  If
> so, that should work.

Before this weekend, I can finish a new patch set with (1) adding a sanity
check and (2) fixing bigalloc bug.  For adding a new lock, it is
unstable now.  So this lock will not be included in this patch set.

Regards,
Zheng
--
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
Zheng Liu - Sept. 26, 2012, 3:59 a.m.
On Tue, Sep 25, 2012 at 11:46:37PM -0400, Theodore Ts'o wrote:
> On Wed, Sep 26, 2012 at 11:54:48AM +0800, Zheng Liu wrote:
> > 
> > Before this weekend, I can finish a new patch set with (1) adding a sanity
> > check and (2) fixing bigalloc bug.  For adding a new lock, it is
> > unstable now.  So this lock will not be included in this patch set.
> 
> That sounds good to me.  Adding a rw lock is something we can do in a
> separate patch (which might not make the merge window for 3.7,
> depending on when Linus decides to release 3.6, but that's OK).

OK, I will submit a new patch set ASAP.

Regards,
Zheng
--
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
Zheng Liu - Sept. 26, 2012, 8 a.m.
On Tue, Sep 25, 2012 at 04:59:21PM -0400, Theodore Ts'o wrote:
> On Tue, Sep 25, 2012 at 08:42:52PM +0800, Zheng Liu wrote:
> > > If so, we might want to think about adding a sanity check to make sure
> > > that by the time we are done with the inode in ext4_evict_inode()
> > > (after we have forced writeback), the ext4_es_tree is empty.  Agreed?
> > 
> > Today I revise this patch again, and I find extent_status_tree is freed
> > in ext4_clear_inode().  So maybe I don't think that we need to check
> > this tree to be freed in ext4_evict_inode().  This change is in this
> > patch '[RFC][PATCH 4/8 v2] ext4: let ext4 maintain extent status tree'.
> > What's your opinion?
> 
> When you say "revise this patch again", does that mean that you would
> like to submit a new set of patch series with changes?  Or just that
> you are looking at this patch set again?
> 
> It's certainly true that ext4_evict_inode() will call
> ext4_clear_inode(), so it's not a question of worrying about a memory
> leak.  I was thinking more about doing this as a cheap sanity check
> for the data structure.  By the time we call ext4_evict_inode(), the
> mm layer all writeback should be complete.  Hence, all of the entries
> to the tree _should_ have been removed by the time we call
> ext4_evict_inode().
> 
> I don't know if this is going to change as you start using this data
> structure for other purposes (such as locking a range of pages), but
> if I understand how things are currently working, it _should_ be the
> case that when ext4_evict_inode() calls ext4_clear_inode(), the call
> to ext4_es_remove_extent() should be a no-op, since all of the nodes
> in the extent status tree should have been released by then.  If it
> isn't, then either I'm not understanding the code, or there's a bug in
> the code.

Hi Ted,

When I try to add a BUG_ON in ext4_evict_inode() to ensure that extent
status tree is empty, I will get an error, which reports that the tree
is not empty.  I find that there still has some dirty pages when we
call ext4_evict_inode() because vfs doesn't write all dirty pages back.
In iput_final(), we firstly call ext4_drop_inode().  If the return value
is true, we won't call write_inode_now() to do a writeback.  It means
that we write some data to a file and remove it immediately, and then
all dirty pages that aren't during writeback progress won't be written.
These pages will be truncated in ext4_evict_inode().  Thus, the extent
status tree is possible not to be empty when we call ext4_evict_inode().
So we cannot add a sanity check in this function.  Am I missing something?

Regards,
Zheng
--
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
Zheng Liu - Sept. 28, 2012, 7:27 a.m.
On Tue, Sep 25, 2012 at 11:37:40PM -0400, Theodore Ts'o wrote:
> On Wed, Sep 26, 2012 at 11:24:26AM +0800, Zheng Liu wrote:
> > > Can you let me know what changes you need to make?  If it is to add
> > > new features or new sanity checks, does it make sense to simply make
> > > it as new commits to existing patch set?  Or are there fundamental
> > > problems with the current set, that would be better to fix in the
> > > current set of commits?  (Or is it just minor stylistic/spelling
> > > fixes?)
> > > 
> > > Thanks!!
> > 
> > In new patch set, there is three changes as beblow:
> > 
> > 1. add a sanity check in ext4_evict_inode()
> > 2. fix a bug in ext4_find_delalloc_range().  This bug is reported by
> > xfstest #230 when we enable bigalloc feature.
> > 3. Add a new rwlock to protect extent status tree.
> > 
> > So I think that we can only add a sanity check and fix the bigalloc bug,
> > and then apply this patch set because the changes are minor.  For adding
> > a new lock to protect extent status tree, we can add this feature in a
> > new patch.  If you think it is OK, I can generate a new patch set, do
> > some tests using xfstest, and submit it as soon as possible.  What's
> > your opinion?
> 
> Do you think you can get me the patches by the end of the week?  If
> so, that should work.

Hi Ted,

Until now, I have fixed the bigalloc bug that is reported by xfstest
#230, and merged Hugh's patch.  But I do really think that this patch
set couldn't be applied at this merge window because the change is not
*minor*, and it still needs to do more tests.  That would be great if
you can keep this patch set in dev branch at this merge window.  Thanks!

Regards,
Zheng
--
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
Theodore Ts'o - Sept. 28, 2012, 5:42 p.m.
On Fri, Sep 28, 2012 at 03:27:14PM +0800, Zheng Liu wrote:
> 
> Until now, I have fixed the bigalloc bug that is reported by xfstest
> #230, and merged Hugh's patch.  But I do really think that this patch
> set couldn't be applied at this merge window because the change is not
> *minor*, and it still needs to do more tests.  That would be great if
> you can keep this patch set in dev branch at this merge window.  Thanks!

The dev branch is the set of patches that are planned to go to Linus
during the next merge window, so if we drop it from the merge window,
I would drop it from the dev branch and put it in the "unstable"
portion of the patch series.

It would be a shame to drop it since this provides the SEEK_HOLE
capability, though.  Can you say more about which change is not minor?
The change to fix the bigalloc bug?   Or the whole patch series?

					- 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
Theodore Ts'o - Sept. 29, 2012, 3:07 a.m.
On Sat, Sep 29, 2012 at 08:54:47AM +0800, Zheng Liu wrote:
> 
> When I try to fix the bigalloc bug, some code that operates on extent
> status tree and  maintains its status are changed when I do some changes in
> ext4_find_delalloc_range().  So that quite has a lot of changes I thought
> in the whole patch series.  So I think that we'd better drop this patch set
> from dev branch.  Thanks.

OK, dropped.

					- 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
Zheng Liu - Sept. 29, 2012, 1:24 p.m.
[Sorry, I got an delivery error when the original mail is sent to the
mailing list.  Cc' to them]

On Saturday, September 29, 2012, Theodore Ts'o wrote:
>
> On Fri, Sep 28, 2012 at 03:27:14PM +0800, Zheng Liu wrote:
> >
> > Until now, I have fixed the bigalloc bug that is reported by xfstest
> > #230, and merged Hugh's patch.  But I do really think that this patch
> > set couldn't be applied at this merge window because the change is not
> > *minor*, and it still needs to do more tests.  That would be great if
> > you can keep this patch set in dev branch at this merge window.  Thanks!
>
> The dev branch is the set of patches that are planned to go to Linus
> during the next merge window, so if we drop it from the merge window,
> I would drop it from the dev branch and put it in the "unstable"
> portion of the patch series.
>
> It would be a shame to drop it since this provides the SEEK_HOLE
> capability, though.  Can you say more about which change is not minor?
> The change to fix the bigalloc bug?   Or the whole patch series?
>
>

Hi Ted,

When I try to fix the bigalloc bug, some code that operates on extent
status tree and  maintains its status are changed when I do some
changes in ext4_find_delalloc_range().  So that quite has a lot of
changes I thought in the whole patch series.  So I think that we'd
better drop this patch set from dev branch.  Thanks.

Regards,
Zheng
--
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
Zheng Liu - Sept. 29, 2012, 1:26 p.m.
On Sat, Sep 29, 2012 at 11:07 AM, Theodore Ts'o <tytso@mit.edu> wrote:
> On Sat, Sep 29, 2012 at 08:54:47AM +0800, Zheng Liu wrote:
>>
>> When I try to fix the bigalloc bug, some code that operates on extent
>> status tree and  maintains its status are changed when I do some changes in
>> ext4_find_delalloc_range().  So that quite has a lot of changes I thought
>> in the whole patch series.  So I think that we'd better drop this patch set
>> from dev branch.  Thanks.
>
> OK, dropped.

Thank you very much.  I will add rwlock to protect this tree, and do
more tests for this patch set.

Regards,
Zheng
--
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
Dmitri Monakho - Sept. 30, 2012, 2 p.m.
On Fri, 28 Sep 2012 23:07:59 -0400, Theodore Ts'o <tytso@mit.edu> wrote:
> On Sat, Sep 29, 2012 at 08:54:47AM +0800, Zheng Liu wrote:
> > 
> > When I try to fix the bigalloc bug, some code that operates on extent
> > status tree and  maintains its status are changed when I do some changes in
> > ext4_find_delalloc_range().  So that quite has a lot of changes I thought
> > in the whole patch series.  So I think that we'd better drop this patch set
> > from dev branch.  Thanks.
> 
> OK, dropped.
After patch-set was dropped, 269'th succeed.
Both warnings: ext4_convert_unwritten_extent which fail with ENOSPC and
WARNING: at fs/ext4/inode.c:362 ext4_da_update_reserve_space
are gone away.  
> 
> 					- Ted
> --
> To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
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

Patch

diff --git a/fs/ext4/super.c b/fs/ext4/super.c
index 3e0851e..353b1fd 100644
--- a/fs/ext4/super.c
+++ b/fs/ext4/super.c
@@ -944,6 +944,7 @@  static struct inode *ext4_alloc_inode(struct super_block *sb)
 	memset(&ei->i_cached_extent, 0, sizeof(struct ext4_ext_cache));
 	INIT_LIST_HEAD(&ei->i_prealloc_list);
 	spin_lock_init(&ei->i_prealloc_lock);
+	ext4_es_init_tree(&ei->i_es_tree);
 	ei->i_reserved_data_blocks = 0;
 	ei->i_reserved_meta_blocks = 0;
 	ei->i_allocated_meta_blocks = 0;