diff mbox

Lucid SRU commit b3214b2a5dcee0d3ba0bd28dc3dd9ca472f3d6df

Message ID 4F5FFB82.9070106@canonical.com
State New
Headers show

Commit Message

Colin Ian King March 14, 2012, 1:59 a.m. UTC 
Hi there,

While running a bunch of exhaustive eCryptfs tests this evening I found 
that one of the recent commits in Lucid master-next was causing a 
regression.  I bisected this down to the offending problem - the 
regression is in lucid master-next because of commit 
b3214b2a5dcee0d3ba0bd28dc3dd9ca472f3d6df

Now, the patch I sent to the mailing list on the 15th March effectively 
modified the dir_fops as follows:



which explains the regression I'm seeing where I can't mmap a file on 
ecryptfs. Pulling out this commit makes the regression go away.

So..

Can we revert b3214b2a5dcee0d3ba0bd28dc3dd9ca472f3d6df and apply my 
original patch and make sure it correctly modifies ecryptfs_dir_fops.

Since it's now 1:56am I'm not really awake to figure out why the patch 
didn't apply correctly.  I can't believe git am failed like this.

Colin

Comments

Tim Gardner March 14, 2012, 3:08 a.m. UTC | #1 
On 03/13/2012 07:59 PM, Colin Ian King wrote:
> Hi there,
>
> While running a bunch of exhaustive eCryptfs tests this evening I found
> that one of the recent commits in Lucid master-next was causing a
> regression. I bisected this down to the offending problem - the
> regression is in lucid master-next because of commit
> b3214b2a5dcee0d3ba0bd28dc3dd9ca472f3d6df
>
> Now, the patch I sent to the mailing list on the 15th March effectively
> modified the dir_fops as follows:
>
> diff --git a/fs/ecryptfs/file.c b/fs/ecryptfs/file.c
> index 4e25328..d8adc51 100644
> --- a/fs/ecryptfs/file.c
> +++ b/fs/ecryptfs/file.c
> @@ -327,7 +327,6 @@ const struct file_operations ecryptfs_dir_fops = {
> #ifdef CONFIG_COMPAT
> .compat_ioctl = ecryptfs_compat_ioctl,
> #endif
> - .mmap = generic_file_mmap,
> .open = ecryptfs_open,
> .flush = ecryptfs_flush,
> .release = ecryptfs_release,
>
> However, the patch applied to lucid master next is different:
>
> diff --git a/fs/ecryptfs/file.c b/fs/ecryptfs/file.c
> index 502b09f..49129f4 100644
> --- a/fs/ecryptfs/file.c
> +++ b/fs/ecryptfs/file.c
> @@ -348,7 +348,6 @@ const struct file_operations ecryptfs_main_fops = {
> #ifdef CONFIG_COMPAT
> .compat_ioctl = ecryptfs_compat_ioctl,
> #endif
> - .mmap = generic_file_mmap,
> .open = ecryptfs_open,
> .flush = ecryptfs_flush,
> .release = ecryptfs_release,
>
>
> which explains the regression I'm seeing where I can't mmap a file on
> ecryptfs. Pulling out this commit makes the regression go away.
>
> So..
>
> Can we revert b3214b2a5dcee0d3ba0bd28dc3dd9ca472f3d6df and apply my
> original patch and make sure it correctly modifies ecryptfs_dir_fops.
>
> Since it's now 1:56am I'm not really awake to figure out why the patch
> didn't apply correctly. I can't believe git am failed like this.
>
> Colin
>

OK, this is a bit weird. lucid master-next 
a0a7873ea78a0c7331e59fe63f20f80c8131b3e8 does the right thing. However, 
a bit later b3214b2a5dcee0d3ba0bd28dc3dd9ca472f3d6df removes the same 
helper from ecryptfs_main_fops. That commit was part of the 2.6.32.58 
stable update. I am totally baffled as to how that happened, but it kind 
of looks like a 'git am' failure. Upstream stable 2.6.32.y looks correct.

Do you think it sufficient to just revert 
b3214b2a5dcee0d3ba0bd28dc3dd9ca472f3d6df ?

rtg
Stefan Bader March 14, 2012, 8:39 a.m. UTC | #2 
On 14.03.2012 04:08, Tim Gardner wrote:
> On 03/13/2012 07:59 PM, Colin Ian King wrote:
>> Hi there,
>>
>> While running a bunch of exhaustive eCryptfs tests this evening I found
>> that one of the recent commits in Lucid master-next was causing a
>> regression. I bisected this down to the offending problem - the
>> regression is in lucid master-next because of commit
>> b3214b2a5dcee0d3ba0bd28dc3dd9ca472f3d6df
>>
>> Now, the patch I sent to the mailing list on the 15th March effectively
>> modified the dir_fops as follows:
>>
>> diff --git a/fs/ecryptfs/file.c b/fs/ecryptfs/file.c
>> index 4e25328..d8adc51 100644
>> --- a/fs/ecryptfs/file.c
>> +++ b/fs/ecryptfs/file.c
>> @@ -327,7 +327,6 @@ const struct file_operations ecryptfs_dir_fops = {
>> #ifdef CONFIG_COMPAT
>> .compat_ioctl = ecryptfs_compat_ioctl,
>> #endif
>> - .mmap = generic_file_mmap,
>> .open = ecryptfs_open,
>> .flush = ecryptfs_flush,
>> .release = ecryptfs_release,
>>
>> However, the patch applied to lucid master next is different:
>>
>> diff --git a/fs/ecryptfs/file.c b/fs/ecryptfs/file.c
>> index 502b09f..49129f4 100644
>> --- a/fs/ecryptfs/file.c
>> +++ b/fs/ecryptfs/file.c
>> @@ -348,7 +348,6 @@ const struct file_operations ecryptfs_main_fops = {
>> #ifdef CONFIG_COMPAT
>> .compat_ioctl = ecryptfs_compat_ioctl,
>> #endif
>> - .mmap = generic_file_mmap,
>> .open = ecryptfs_open,
>> .flush = ecryptfs_flush,
>> .release = ecryptfs_release,
>>
>>
>> which explains the regression I'm seeing where I can't mmap a file on
>> ecryptfs. Pulling out this commit makes the regression go away.
>>
>> So..
>>
>> Can we revert b3214b2a5dcee0d3ba0bd28dc3dd9ca472f3d6df and apply my
>> original patch and make sure it correctly modifies ecryptfs_dir_fops.
>>
>> Since it's now 1:56am I'm not really awake to figure out why the patch
>> didn't apply correctly. I can't believe git am failed like this.
>>
>> Colin
>>
> 
> OK, this is a bit weird. lucid master-next
> a0a7873ea78a0c7331e59fe63f20f80c8131b3e8 does the right thing. However, a bit
> later b3214b2a5dcee0d3ba0bd28dc3dd9ca472f3d6df removes the same helper from
> ecryptfs_main_fops. That commit was part of the 2.6.32.58 stable update. I am
> totally baffled as to how that happened, but it kind of looks like a 'git am'
> failure. Upstream stable 2.6.32.y looks correct.
> 
> Do you think it sufficient to just revert
> b3214b2a5dcee0d3ba0bd28dc3dd9ca472f3d6df ?
> 
> rtg
> 
This had happened to me once in the past, just with an addition instead of a
removal. If the hunk modified is not differing enough in context (or in this
case there is another place that fits as well), it is possible to not detect an
already applied patch.
And this one does and we had it before and after. So probably the simplest is to
rebase the second (wrong) one out of existence.

-Stefan
Colin Ian King March 14, 2012, 9:01 a.m. UTC | #3 
On 14/03/12 08:39, Stefan Bader wrote:
> On 14.03.2012 04:08, Tim Gardner wrote:
>> On 03/13/2012 07:59 PM, Colin Ian King wrote:
>>> Hi there,
>>>
>>> While running a bunch of exhaustive eCryptfs tests this evening I found
>>> that one of the recent commits in Lucid master-next was causing a
>>> regression. I bisected this down to the offending problem - the
>>> regression is in lucid master-next because of commit
>>> b3214b2a5dcee0d3ba0bd28dc3dd9ca472f3d6df
>>>
>>> Now, the patch I sent to the mailing list on the 15th March effectively
>>> modified the dir_fops as follows:
>>>
>>> diff --git a/fs/ecryptfs/file.c b/fs/ecryptfs/file.c
>>> index 4e25328..d8adc51 100644
>>> --- a/fs/ecryptfs/file.c
>>> +++ b/fs/ecryptfs/file.c
>>> @@ -327,7 +327,6 @@ const struct file_operations ecryptfs_dir_fops = {
>>> #ifdef CONFIG_COMPAT
>>> .compat_ioctl = ecryptfs_compat_ioctl,
>>> #endif
>>> - .mmap = generic_file_mmap,
>>> .open = ecryptfs_open,
>>> .flush = ecryptfs_flush,
>>> .release = ecryptfs_release,
>>>
>>> However, the patch applied to lucid master next is different:
>>>
>>> diff --git a/fs/ecryptfs/file.c b/fs/ecryptfs/file.c
>>> index 502b09f..49129f4 100644
>>> --- a/fs/ecryptfs/file.c
>>> +++ b/fs/ecryptfs/file.c
>>> @@ -348,7 +348,6 @@ const struct file_operations ecryptfs_main_fops = {
>>> #ifdef CONFIG_COMPAT
>>> .compat_ioctl = ecryptfs_compat_ioctl,
>>> #endif
>>> - .mmap = generic_file_mmap,
>>> .open = ecryptfs_open,
>>> .flush = ecryptfs_flush,
>>> .release = ecryptfs_release,
>>>
>>>
>>> which explains the regression I'm seeing where I can't mmap a file on
>>> ecryptfs. Pulling out this commit makes the regression go away.
>>>
>>> So..
>>>
>>> Can we revert b3214b2a5dcee0d3ba0bd28dc3dd9ca472f3d6df and apply my
>>> original patch and make sure it correctly modifies ecryptfs_dir_fops.
>>>
>>> Since it's now 1:56am I'm not really awake to figure out why the patch
>>> didn't apply correctly. I can't believe git am failed like this.
>>>
>>> Colin
>>>
>>
>> OK, this is a bit weird. lucid master-next
>> a0a7873ea78a0c7331e59fe63f20f80c8131b3e8 does the right thing. However, a bit
>> later b3214b2a5dcee0d3ba0bd28dc3dd9ca472f3d6df removes the same helper from
>> ecryptfs_main_fops. That commit was part of the 2.6.32.58 stable update. I am
>> totally baffled as to how that happened, but it kind of looks like a 'git am'
>> failure. Upstream stable 2.6.32.y looks correct.
>>
>> Do you think it sufficient to just revert
>> b3214b2a5dcee0d3ba0bd28dc3dd9ca472f3d6df ?
>>
>> rtg
>>
> This had happened to me once in the past, just with an addition instead of a
> removal. If the hunk modified is not differing enough in context (or in this
> case there is another place that fits as well), it is possible to not detect an
> already applied patch.
> And this one does and we had it before and after. So probably the simplest is to
> rebase the second (wrong) one out of existence.
+1 on that

Colin
>
> -Stefan
>
Andy Whitcroft March 14, 2012, 9:19 a.m. UTC | #4 
On Tue, Mar 13, 2012 at 09:08:55PM -0600, Tim Gardner wrote:
> On 03/13/2012 07:59 PM, Colin Ian King wrote:
> >Hi there,
> >
> >While running a bunch of exhaustive eCryptfs tests this evening I found
> >that one of the recent commits in Lucid master-next was causing a
> >regression. I bisected this down to the offending problem - the
> >regression is in lucid master-next because of commit
> >b3214b2a5dcee0d3ba0bd28dc3dd9ca472f3d6df
> >
> >Now, the patch I sent to the mailing list on the 15th March effectively
> >modified the dir_fops as follows:
> >
> >diff --git a/fs/ecryptfs/file.c b/fs/ecryptfs/file.c
> >index 4e25328..d8adc51 100644
> >--- a/fs/ecryptfs/file.c
> >+++ b/fs/ecryptfs/file.c
> >@@ -327,7 +327,6 @@ const struct file_operations ecryptfs_dir_fops = {
> >#ifdef CONFIG_COMPAT
> >.compat_ioctl = ecryptfs_compat_ioctl,
> >#endif
> >- .mmap = generic_file_mmap,
> >.open = ecryptfs_open,
> >.flush = ecryptfs_flush,
> >.release = ecryptfs_release,
> >
> >However, the patch applied to lucid master next is different:
> >
> >diff --git a/fs/ecryptfs/file.c b/fs/ecryptfs/file.c
> >index 502b09f..49129f4 100644
> >--- a/fs/ecryptfs/file.c
> >+++ b/fs/ecryptfs/file.c
> >@@ -348,7 +348,6 @@ const struct file_operations ecryptfs_main_fops = {
> >#ifdef CONFIG_COMPAT
> >.compat_ioctl = ecryptfs_compat_ioctl,
> >#endif
> >- .mmap = generic_file_mmap,
> >.open = ecryptfs_open,
> >.flush = ecryptfs_flush,
> >.release = ecryptfs_release,
> >
> >
> >which explains the regression I'm seeing where I can't mmap a file on
> >ecryptfs. Pulling out this commit makes the regression go away.
> >
> >So..
> >
> >Can we revert b3214b2a5dcee0d3ba0bd28dc3dd9ca472f3d6df and apply my
> >original patch and make sure it correctly modifies ecryptfs_dir_fops.
> >
> >Since it's now 1:56am I'm not really awake to figure out why the patch
> >didn't apply correctly. I can't believe git am failed like this.
> >
> >Colin
> >
> 
> OK, this is a bit weird. lucid master-next
> a0a7873ea78a0c7331e59fe63f20f80c8131b3e8 does the right thing.
> However, a bit later b3214b2a5dcee0d3ba0bd28dc3dd9ca472f3d6df
> removes the same helper from ecryptfs_main_fops. That commit was
> part of the 2.6.32.58 stable update. I am totally baffled as to how
> that happened, but it kind of looks like a 'git am' failure.
> Upstream stable 2.6.32.y looks correct.
> 
> Do you think it sufficient to just revert
> b3214b2a5dcee0d3ba0bd28dc3dd9ca472f3d6df ?

Ok what happened here is that there are two places this patch can apply,
ie where the hunk context is identicle.  So colin's backport applied in
the right place, removing the first match.  When the upstream stable
version gets applied the original location does not match but the later
one matches exactly so it is applied with an offset, removing the wrong
one.

Tim, yes just reverting b3214b2a5dcee0d3ba0bd28dc3dd9ca472f3d6df should
sort things out, it is that fix which is now in error.

-apw
Andy Whitcroft March 14, 2012, 9:31 a.m. UTC | #5 
On Wed, Mar 14, 2012 at 09:19:18AM +0000, Andy Whitcroft wrote:

> Tim, yes just reverting b3214b2a5dcee0d3ba0bd28dc3dd9ca472f3d6df should
> sort things out, it is that fix which is now in error.

For now I have reverted this on the tip of lucid/master-next and asked
Colin to confirm it fixes everything for him (which it should).  It can
then be rebased out of existance.

-apw
Colin Ian King March 14, 2012, 10:15 a.m. UTC | #6 
On 14/03/12 09:31, Andy Whitcroft wrote:
> On Wed, Mar 14, 2012 at 09:19:18AM +0000, Andy Whitcroft wrote:
>
>> Tim, yes just reverting b3214b2a5dcee0d3ba0bd28dc3dd9ca472f3d6df should
>> sort things out, it is that fix which is now in error.
>
> For now I have reverted this on the tip of lucid/master-next and asked
> Colin to confirm it fixes everything for him (which it should).  It can
> then be rebased out of existence.

I've given this a test and can confirm this fixes the issue.

Colin

>
> -apw
>
Tim Gardner March 14, 2012, 11:52 a.m. UTC | #7 
On 03/14/2012 04:15 AM, Colin Ian King wrote:
> On 14/03/12 09:31, Andy Whitcroft wrote:
>> On Wed, Mar 14, 2012 at 09:19:18AM +0000, Andy Whitcroft wrote:
>>
>>> Tim, yes just reverting b3214b2a5dcee0d3ba0bd28dc3dd9ca472f3d6df should
>>> sort things out, it is that fix which is now in error.
>>
>> For now I have reverted this on the tip of lucid/master-next and asked
>> Colin to confirm it fixes everything for him (which it should). It can
>> then be rebased out of existence.
>
> I've given this a test and can confirm this fixes the issue.
>
> Colin
>
>>
>> -apw
>>
>

Done
diff mbox

Patch

diff --git a/fs/ecryptfs/file.c b/fs/ecryptfs/file.c
index 4e25328..d8adc51 100644
--- a/fs/ecryptfs/file.c
+++ b/fs/ecryptfs/file.c
@@ -327,7 +327,6 @@  const struct file_operations ecryptfs_dir_fops = {
  #ifdef CONFIG_COMPAT
  	.compat_ioctl = ecryptfs_compat_ioctl,
  #endif
-	.mmap = generic_file_mmap,
  	.open = ecryptfs_open,
  	.flush = ecryptfs_flush,
  	.release = ecryptfs_release,

However, the patch applied to lucid master next is different:

diff --git a/fs/ecryptfs/file.c b/fs/ecryptfs/file.c
index 502b09f..49129f4 100644
--- a/fs/ecryptfs/file.c
+++ b/fs/ecryptfs/file.c
@@ -348,7 +348,6 @@  const struct file_operations ecryptfs_main_fops = {
  #ifdef CONFIG_COMPAT
         .compat_ioctl = ecryptfs_compat_ioctl,
  #endif
-       .mmap = generic_file_mmap,
         .open = ecryptfs_open,
         .flush = ecryptfs_flush,
         .release = ecryptfs_release,