| Message ID | 1304956630-20384-4-git-send-email-lczerner@redhat.com |
|---|---|
| State | Superseded, archived |
| Headers | show |
On 5/9/11 10:57 AM, Lukas Czerner wrote: > We need to take reference to the s_li_request after we take a mutex, > because it might be freed since then, hence result in accessing old > already freed memory. Also we should protect the whole > ext4_remove_li_request() because ext4_li_info might be in the process of > being freed in ext4_lazyinit_thread(). It'd be really great to have some comments which explain just what ext4_li_mtx protects, but I'm working on an add-comments patch for the lazyinit stuff (I commented things a bit as I reviewed your changes) so I'll send that along later. in any case, the change looks ok, thanks. Reviewed-by: Eric Sandeen <sandeen@redhat.com> > Signed-off-by: Lukas Czerner <lczerner@redhat.com> > --- > fs/ext4/super.c | 10 ++++++---- > 1 files changed, 6 insertions(+), 4 deletions(-) > > diff --git a/fs/ext4/super.c b/fs/ext4/super.c > index c379af6..6a8e48f 100644 > --- a/fs/ext4/super.c > +++ b/fs/ext4/super.c > @@ -2721,14 +2721,16 @@ static void ext4_remove_li_request(struct ext4_li_request *elr) > > static void ext4_unregister_li_request(struct super_block *sb) > { > - struct ext4_li_request *elr = EXT4_SB(sb)->s_li_request; > - > - if (!ext4_li_info) > + mutex_lock(&ext4_li_mtx); > + if (!ext4_li_info) { > + mutex_unlock(&ext4_li_mtx); > return; > + } > > mutex_lock(&ext4_li_info->li_list_mtx); > - ext4_remove_li_request(elr); > + ext4_remove_li_request(EXT4_SB(sb)->s_li_request); > mutex_unlock(&ext4_li_info->li_list_mtx); > + mutex_unlock(&ext4_li_mtx); > } > > static struct task_struct *ext4_lazyinit_task; -- 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 Thu, 19 May 2011, Eric Sandeen wrote: > On 5/9/11 10:57 AM, Lukas Czerner wrote: > > We need to take reference to the s_li_request after we take a mutex, > > because it might be freed since then, hence result in accessing old > > already freed memory. Also we should protect the whole > > ext4_remove_li_request() because ext4_li_info might be in the process of > > being freed in ext4_lazyinit_thread(). > > It'd be really great to have some comments which explain just what > ext4_li_mtx protects, but I'm working on an add-comments patch for > the lazyinit stuff (I commented things a bit as I reviewed your > changes) so I'll send that along later. The ext4_li_mtx is protecting the whole ext4_li_info structure so it can be atomically created and freed. > > in any case, the change looks ok, thanks. > > Reviewed-by: Eric Sandeen <sandeen@redhat.com> Thanks! -Lukas > > > > Signed-off-by: Lukas Czerner <lczerner@redhat.com> > > --- > > fs/ext4/super.c | 10 ++++++---- > > 1 files changed, 6 insertions(+), 4 deletions(-) > > > > diff --git a/fs/ext4/super.c b/fs/ext4/super.c > > index c379af6..6a8e48f 100644 > > --- a/fs/ext4/super.c > > +++ b/fs/ext4/super.c > > @@ -2721,14 +2721,16 @@ static void ext4_remove_li_request(struct ext4_li_request *elr) > > > > static void ext4_unregister_li_request(struct super_block *sb) > > { > > - struct ext4_li_request *elr = EXT4_SB(sb)->s_li_request; > > - > > - if (!ext4_li_info) > > + mutex_lock(&ext4_li_mtx); > > + if (!ext4_li_info) { > > + mutex_unlock(&ext4_li_mtx); > > return; > > + } > > > > mutex_lock(&ext4_li_info->li_list_mtx); > > - ext4_remove_li_request(elr); > > + ext4_remove_li_request(EXT4_SB(sb)->s_li_request); > > mutex_unlock(&ext4_li_info->li_list_mtx); > > + mutex_unlock(&ext4_li_mtx); > > } > > > > static struct task_struct *ext4_lazyinit_task; > > -- 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
Lukas, are you going to be providing a new version of these patch series or not? If you are, could you do it as a separate patch series, instead of only updating one patch as a reply to the mail thread. When people do this, I find it painful since I need to figure out, "ok, I need v2 of the 1/4 patch, v3 of the 2/4 patch, v4 of the 3/4 patch, and v3 of of the 4/4 patch. To provide context, please add version descriptors after the --- of the patch. (i.e, "v3 --> v4; fixed commit message") Also, if we're going to be doing extended review of patches like this, instead of my just fixing things up when I pull stuff in, people need to start authoring patches ***much*** sooner. Doing multiple publish and review cycles now that the merge window is open really doesn't work. One way of solving this in the future is to simply not take any patch that is first submitted after say, -rc5 or -rc6 until the next merge window. But given that some patches didn't *start* getting much review until 2-3 weeks ago, that wouldn't be entirely fair. But for the next merge window, if this is going to work, we need people submitting patches earlier, and people reviewing patches earlier. 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
On 5/20/11 11:03 AM, Ted Ts'o wrote: > Lukas, are you going to be providing a new version of these patch > series or not? > > If you are, could you do it as a separate patch series, instead of > only updating one patch as a reply to the mail thread. When people do > this, I find it painful since I need to figure out, "ok, I need v2 of > the 1/4 patch, v3 of the 2/4 patch, v4 of the 3/4 patch, and v3 of of > the 4/4 patch. To provide context, please add version descriptors > after the --- of the patch. (i.e, "v3 --> v4; fixed commit message") > > Also, if we're going to be doing extended review of patches like this, > instead of my just fixing things up when I pull stuff in, people need > to start authoring patches ***much*** sooner. Doing multiple publish > and review cycles now that the merge window is open really doesn't > work. One way of solving this in the future is to simply not take any > patch that is first submitted after say, -rc5 or -rc6 until the next > merge window. But given that some patches didn't *start* getting much > review until 2-3 weeks ago, that wouldn't be entirely fair. > > But for the next merge window, if this is going to work, we need > people submitting patches earlier, and people reviewing patches > earlier. How about a reasonable sounding convention like: if it's non-critical it's too late, but if it's critical you'll try to get it in. Windows are windows, reviews are reviews, and if it's too late, it's too late ... You ultimately get to decide what goes in and when. But skipping thorough review simply because the window is open now doesn't make sense to me. -Eric -- 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
Hi Ted, On Fri, 20 May 2011, Ted Ts'o wrote: > Lukas, are you going to be providing a new version of these patch > series or not? I already did send it as a separate patch series with the version description. > > If you are, could you do it as a separate patch series, instead of > only updating one patch as a reply to the mail thread. When people do > this, I find it painful since I need to figure out, "ok, I need v2 of > the 1/4 patch, v3 of the 2/4 patch, v4 of the 3/4 patch, and v3 of of > the 4/4 patch. To provide context, please add version descriptors > after the --- of the patch. (i.e, "v3 --> v4; fixed commit message") > > Also, if we're going to be doing extended review of patches like this, > instead of my just fixing things up when I pull stuff in, people need > to start authoring patches ***much*** sooner. Doing multiple publish > and review cycles now that the merge window is open really doesn't > work. One way of solving this in the future is to simply not take any > patch that is first submitted after say, -rc5 or -rc6 until the next > merge window. But given that some patches didn't *start* getting much > review until 2-3 weeks ago, that wouldn't be entirely fair. We all should do better, but I am not sure that limits like that are very useful. Some level of flexibility is always needed. > > But for the next merge window, if this is going to work, we need > people submitting patches earlier, and people reviewing patches > earlier. So let's to that people ;). > > Thanks, > > - Ted Thanks Ted! -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 Fri, May 20, 2011 at 06:16:34PM +0200, Lukas Czerner wrote: > > I already did send it as a separate patch series with the version > description. What, you mean the -v2 patch series? But Eric has made further suggestions where you said you might try to do more work. Or did I misread the mail thread? This is why I hate it when you reply to the mail thread with an updated patch. It makes it very hard to figure out what's going on.... So let me rephrase the question: Is -v2 your final answer? - 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, May 20, 2011 at 01:39:46PM -0400, Ted Ts'o wrote: > On Fri, May 20, 2011 at 06:16:34PM +0200, Lukas Czerner wrote: > > > > I already did send it as a separate patch series with the version > > description. > > What, you mean the -v2 patch series? > > > So let me rephrase the question: Is -v2 your final answer? Ah, never mind. I got confused when with the other patch series where half the patches where -v3, and the other half were -v2. It makes what shows up in patchwork hard to follow. In the future, please bump the version number on the entire series, not just on individual patches, as it makes it a lot easier for me to track what is going on.... - 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, May 20, 2011 at 11:12:05AM -0500, Eric Sandeen wrote: > > But skipping thorough review simply because the window is open now > doesn't make sense to me. Sure, but adding a 24-hour turnaround for at least simple fix ups (commit descriptions, comments, white space, etc) doesn't make a lot of sense either. - 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 5/20/11 12:47 PM, Ted Ts'o wrote: > On Fri, May 20, 2011 at 11:12:05AM -0500, Eric Sandeen wrote: >> >> But skipping thorough review simply because the window is open now >> doesn't make sense to me. > > Sure, but adding a 24-hour turnaround for at least simple fix ups > (commit descriptions, comments, white space, etc) doesn't make a lot > of sense either. > > - Ted I think you are free to use your discretion for those things :) I mean if the last review comment is "you have a typo here" and you really want to make the window, it seems quite reasonable to fix it up, with a note in the commit log. -Eric -- 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/super.c b/fs/ext4/super.c index c379af6..6a8e48f 100644 --- a/fs/ext4/super.c +++ b/fs/ext4/super.c @@ -2721,14 +2721,16 @@ static void ext4_remove_li_request(struct ext4_li_request *elr) static void ext4_unregister_li_request(struct super_block *sb) { - struct ext4_li_request *elr = EXT4_SB(sb)->s_li_request; - - if (!ext4_li_info) + mutex_lock(&ext4_li_mtx); + if (!ext4_li_info) { + mutex_unlock(&ext4_li_mtx); return; + } mutex_lock(&ext4_li_info->li_list_mtx); - ext4_remove_li_request(elr); + ext4_remove_li_request(EXT4_SB(sb)->s_li_request); mutex_unlock(&ext4_li_info->li_list_mtx); + mutex_unlock(&ext4_li_mtx); } static struct task_struct *ext4_lazyinit_task;
We need to take reference to the s_li_request after we take a mutex, because it might be freed since then, hence result in accessing old already freed memory. Also we should protect the whole ext4_remove_li_request() because ext4_li_info might be in the process of being freed in ext4_lazyinit_thread(). Signed-off-by: Lukas Czerner <lczerner@redhat.com> --- fs/ext4/super.c | 10 ++++++---- 1 files changed, 6 insertions(+), 4 deletions(-)