XFS hole punch races

XFS hole punch races

[Jan Kara]

Hi,

similarly to ext4 also XFS had races between hole punching and page faults
which could result in data corruption. The fixes were merged in 4.1-rc1 but
it might make sense to backport them to older stable releases given the
nature of the issue.

Relevant fixes are:

de0e8c20ba3a65b0f15040aabbefdc1999876e6b
075a924d45cc69c75a35f20b4912b85aa98b180a
e8e9ad42c1f1e1bfbe0e8c32c8cac02e9ebfb7ef
0f9160b444e4de33b65dfcd3b901358a3129461a
723cac48473358939759885a18e8df113ea96138
ec56b1f1fdc69599963574ce94cc5693d535dd64

								Honza

-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

XFS hole punch races

[Jan Kara]

Hi,

similarly to ext4 also XFS had races between hole punching and page faults
which could result in data corruption. The fixes were merged in 4.1-rc1 but
it might make sense to backport them to older stable releases given the
nature of the issue.

Relevant fixes are:

de0e8c20ba3a65b0f15040aabbefdc1999876e6b
075a924d45cc69c75a35f20b4912b85aa98b180a
e8e9ad42c1f1e1bfbe0e8c32c8cac02e9ebfb7ef
0f9160b444e4de33b65dfcd3b901358a3129461a
723cac48473358939759885a18e8df113ea96138
ec56b1f1fdc69599963574ce94cc5693d535dd64

								Honza

-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: XFS hole punch races

[Greg KH]

On Tue, Mar 22, 2016 at 04:57:40PM +0100, Jan Kara wrote:
> Hi,
> 
> similarly to ext4 also XFS had races between hole punching and page faults
> which could result in data corruption. The fixes were merged in 4.1-rc1 but
> it might make sense to backport them to older stable releases given the
> nature of the issue.
> 
> Relevant fixes are:
> 
> de0e8c20ba3a65b0f15040aabbefdc1999876e6b
> 075a924d45cc69c75a35f20b4912b85aa98b180a
> e8e9ad42c1f1e1bfbe0e8c32c8cac02e9ebfb7ef
> 0f9160b444e4de33b65dfcd3b901358a3129461a
> 723cac48473358939759885a18e8df113ea96138
> ec56b1f1fdc69599963574ce94cc5693d535dd64

None of these apply to 3.14-stable, so I'm not going to worry about them
:)

thanks,

greg k-h

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: XFS hole punch races

[Greg KH]

On Tue, Mar 22, 2016 at 04:57:40PM +0100, Jan Kara wrote:
> Hi,
> 
> similarly to ext4 also XFS had races between hole punching and page faults
> which could result in data corruption. The fixes were merged in 4.1-rc1 but
> it might make sense to backport them to older stable releases given the
> nature of the issue.
> 
> Relevant fixes are:
> 
> de0e8c20ba3a65b0f15040aabbefdc1999876e6b
> 075a924d45cc69c75a35f20b4912b85aa98b180a
> e8e9ad42c1f1e1bfbe0e8c32c8cac02e9ebfb7ef
> 0f9160b444e4de33b65dfcd3b901358a3129461a
> 723cac48473358939759885a18e8df113ea96138
> ec56b1f1fdc69599963574ce94cc5693d535dd64

None of these apply to 3.14-stable, so I'm not going to worry about them
:)

thanks,

greg k-h

Re: XFS hole punch races

[Ben Hutchings]

[-- Attachment #1.1: Type: text/plain, Size: 1195 bytes --]

On Tue, 2016-03-22 at 16:57 +0100, Jan Kara wrote:
> Hi,
> 
> similarly to ext4 also XFS had races between hole punching and page faults
> which could result in data corruption. The fixes were merged in 4.1-rc1 but
> it might make sense to backport them to older stable releases given the
> nature of the issue.
> 
> Relevant fixes are:
> 
> de0e8c20ba3a65b0f15040aabbefdc1999876e6b
> 075a924d45cc69c75a35f20b4912b85aa98b180a
> e8e9ad42c1f1e1bfbe0e8c32c8cac02e9ebfb7ef
> 0f9160b444e4de33b65dfcd3b901358a3129461a
> 723cac48473358939759885a18e8df113ea96138
> ec56b1f1fdc69599963574ce94cc5693d535dd64

Thanks.  For 3.2 I needed to apply these first:

f38996f57687 xfs: reduce ilock hold times in xfs_setattr_size
bc4010ecb8f4 xfs: use iolock on XFS_IOC_ALLOCSP calls
76ca4c238cf5 xfs: always take the iolock around xfs_setattr_size
5f8aca8b43f4 xfs: always hold the iolock when calling xfs_change_file_space
653c60b633a9 xfs: introduce mmap/truncate lock

and I left out 723cac484733 "xfs: lock out page faults from extent swap
operations" as it doesn't seem applicable.

Ben.

-- 
Ben Hutchings
For every action, there is an equal and opposite criticism. - Harrison

[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

[-- Attachment #2: Type: text/plain, Size: 121 bytes --]

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: XFS hole punch races

[Ben Hutchings]

[-- Attachment #1: Type: text/plain, Size: 1195 bytes --]

On Tue, 2016-03-22 at 16:57 +0100, Jan Kara wrote:
> Hi,
> 
> similarly to ext4 also XFS had races between hole punching and page faults
> which could result in data corruption. The fixes were merged in 4.1-rc1 but
> it might make sense to backport them to older stable releases given the
> nature of the issue.
> 
> Relevant fixes are:
> 
> de0e8c20ba3a65b0f15040aabbefdc1999876e6b
> 075a924d45cc69c75a35f20b4912b85aa98b180a
> e8e9ad42c1f1e1bfbe0e8c32c8cac02e9ebfb7ef
> 0f9160b444e4de33b65dfcd3b901358a3129461a
> 723cac48473358939759885a18e8df113ea96138
> ec56b1f1fdc69599963574ce94cc5693d535dd64

Thanks.  For 3.2 I needed to apply these first:

f38996f57687 xfs: reduce ilock hold times in xfs_setattr_size
bc4010ecb8f4 xfs: use iolock on XFS_IOC_ALLOCSP calls
76ca4c238cf5 xfs: always take the iolock around xfs_setattr_size
5f8aca8b43f4 xfs: always hold the iolock when calling xfs_change_file_space
653c60b633a9 xfs: introduce mmap/truncate lock

and I left out 723cac484733 "xfs: lock out page faults from extent swap
operations" as it doesn't seem applicable.

Ben.

-- 
Ben Hutchings
For every action, there is an equal and opposite criticism. - Harrison

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: XFS hole punch races

[Ben Hutchings]

[-- Attachment #1.1: Type: text/plain, Size: 1325 bytes --]

On Tue, 2016-03-22 at 16:57 +0100, Jan Kara wrote:

Hi,

similarly to ext4 also XFS had races between hole punching and page faults
which could result in data corruption. The fixes were merged in 4.1-rc1 but
it might make sense to backport them to older stable releases given the
nature of the issue.

Relevant fixes are:

de0e8c20ba3a65b0f15040aabbefdc1999876e6b
075a924d45cc69c75a35f20b4912b85aa98b180a
e8e9ad42c1f1e1bfbe0e8c32c8cac02e9ebfb7ef
0f9160b444e4de33b65dfcd3b901358a3129461a
723cac48473358939759885a18e8df113ea96138
ec56b1f1fdc69599963574ce94cc5693d535dd64


You missed the first in that sequence:

653c60b633a9 xfs: introduce mmap/truncate lock

For 3.16, I've queued up all those fixes with one further prerequisite:

812176832169 xfs: fix swapext ilock deadlock

For 3.2, I've queued up all but 723cac484733, with these additional
prerequisites:

f38996f57687 xfs: reduce ilock hold times in xfs_setattr_size
bc4010ecb8f4 xfs: use iolock on XFS_IOC_ALLOCSP calls
76ca4c238cf5 xfs: always take the iolock around xfs_setattr_size
5f8aca8b43f4 xfs: always hold the iolock when calling xfs_change_file_space
I realise I'll need to check for regressions with xfstests.

Ben.

-- 
Ben Hutchings
The most exhausting thing in life is being insincere. - Anne Morrow
Lindberg

[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

[-- Attachment #2: Type: text/plain, Size: 121 bytes --]

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: XFS hole punch races

[Ben Hutchings]

[-- Attachment #1: Type: text/plain, Size: 1325 bytes --]

On Tue, 2016-03-22 at 16:57 +0100, Jan Kara wrote:

Hi,

similarly to ext4 also XFS had races between hole punching and page faults
which could result in data corruption. The fixes were merged in 4.1-rc1 but
it might make sense to backport them to older stable releases given the
nature of the issue.

Relevant fixes are:

de0e8c20ba3a65b0f15040aabbefdc1999876e6b
075a924d45cc69c75a35f20b4912b85aa98b180a
e8e9ad42c1f1e1bfbe0e8c32c8cac02e9ebfb7ef
0f9160b444e4de33b65dfcd3b901358a3129461a
723cac48473358939759885a18e8df113ea96138
ec56b1f1fdc69599963574ce94cc5693d535dd64


You missed the first in that sequence:

653c60b633a9 xfs: introduce mmap/truncate lock

For 3.16, I've queued up all those fixes with one further prerequisite:

812176832169 xfs: fix swapext ilock deadlock

For 3.2, I've queued up all but 723cac484733, with these additional
prerequisites:

f38996f57687 xfs: reduce ilock hold times in xfs_setattr_size
bc4010ecb8f4 xfs: use iolock on XFS_IOC_ALLOCSP calls
76ca4c238cf5 xfs: always take the iolock around xfs_setattr_size
5f8aca8b43f4 xfs: always hold the iolock when calling xfs_change_file_space
I realise I'll need to check for regressions with xfstests.

Ben.

-- 
Ben Hutchings
The most exhausting thing in life is being insincere. - Anne Morrow
Lindberg

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: XFS hole punch races

[Dave Chinner]

On Sat, Jun 04, 2016 at 06:11:10PM +0100, Ben Hutchings wrote:
> On Tue, 2016-03-22 at 16:57 +0100, Jan Kara wrote:
> 
> Hi,
> 
> similarly to ext4 also XFS had races between hole punching and page faults
> which could result in data corruption. The fixes were merged in 4.1-rc1 but
> it might make sense to backport them to older stable releases given the
> nature of the issue.
> 
> Relevant fixes are:
> 
> de0e8c20ba3a65b0f15040aabbefdc1999876e6b
> 075a924d45cc69c75a35f20b4912b85aa98b180a
> e8e9ad42c1f1e1bfbe0e8c32c8cac02e9ebfb7ef
> 0f9160b444e4de33b65dfcd3b901358a3129461a
> 723cac48473358939759885a18e8df113ea96138
> ec56b1f1fdc69599963574ce94cc5693d535dd64
> 
> 
> You missed the first in that sequence:
> 
> 653c60b633a9 xfs: introduce mmap/truncate lock
> 
> For 3.16, I've queued up all those fixes with one further prerequisite:
> 
> 812176832169 xfs: fix swapext ilock deadlock
> 
> For 3.2, I've queued up all but 723cac484733, with these additional
> prerequisites:
> 
> f38996f57687 xfs: reduce ilock hold times in xfs_setattr_size
> bc4010ecb8f4 xfs: use iolock on XFS_IOC_ALLOCSP calls
> 76ca4c238cf5 xfs: always take the iolock around xfs_setattr_size
> 5f8aca8b43f4 xfs: always hold the iolock when calling xfs_change_file_space
> I realise I'll need to check for regressions with xfstests.

Hi Ben,

You do realise that this sort of backport effectively makes the
stable kernels unsupportable by the upstream XFS developers? You're
taking random changes from the upstream kernel until the kernel
compiles, and then mostly hoping that it works.

It's trivially easy to break truncate by screwing up the locking and
IO barriers that it depends on, and this set of patches make quite a
lot of different changes that have an unknown set of dependencies
for correct behaviour. xfstests won't find all those problems; at
best it will tell us that there won't be obvious problems.

Stable kernels are supposed to be stable, and backports like this
are riskier than the original changes in the upstream kernels. if a
user is hitting a mmap/hole-punch race on an XFS filesytem (I can't
remember any reports to the upstream list for any kernel) on a 3.2
kernel, then correct answer is "upgrade to a newer upstream kernel",
not "do a risky backport that exposes the entire user base to the
potential of new data corruption bugs"....

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: XFS hole punch races

[Dave Chinner]

On Sat, Jun 04, 2016 at 06:11:10PM +0100, Ben Hutchings wrote:
> On Tue, 2016-03-22 at 16:57 +0100, Jan Kara wrote:
> 
> Hi,
> 
> similarly to ext4 also XFS had races between hole punching and page faults
> which could result in data corruption. The fixes were merged in 4.1-rc1 but
> it might make sense to backport them to older stable releases given the
> nature of the issue.
> 
> Relevant fixes are:
> 
> de0e8c20ba3a65b0f15040aabbefdc1999876e6b
> 075a924d45cc69c75a35f20b4912b85aa98b180a
> e8e9ad42c1f1e1bfbe0e8c32c8cac02e9ebfb7ef
> 0f9160b444e4de33b65dfcd3b901358a3129461a
> 723cac48473358939759885a18e8df113ea96138
> ec56b1f1fdc69599963574ce94cc5693d535dd64
> 
> 
> You missed the first in that sequence:
> 
> 653c60b633a9 xfs: introduce mmap/truncate lock
> 
> For 3.16, I've queued up all�those fixes with one further�prerequisite:
> 
> 812176832169 xfs: fix swapext ilock deadlock
> 
> For 3.2, I've queued up all but 723cac484733, with these additional
> prerequisites:
> 
> f38996f57687 xfs: reduce ilock hold times in xfs_setattr_size
> bc4010ecb8f4 xfs: use iolock on XFS_IOC_ALLOCSP calls
> 76ca4c238cf5 xfs: always take the iolock around xfs_setattr_size
> 5f8aca8b43f4 xfs: always hold the iolock when calling xfs_change_file_space
> I realise I'll need to check for regressions with xfstests.

Hi Ben,

You do realise that this sort of backport effectively makes the
stable kernels unsupportable by the upstream XFS developers? You're
taking random changes from the upstream kernel until the kernel
compiles, and then mostly hoping that it works.

It's trivially easy to break truncate by screwing up the locking and
IO barriers that it depends on, and this set of patches make quite a
lot of different changes that have an unknown set of dependencies
for correct behaviour. xfstests won't find all those problems; at
best it will tell us that there won't be obvious problems.

Stable kernels are supposed to be stable, and backports like this
are riskier than the original changes in the upstream kernels. if a
user is hitting a mmap/hole-punch race on an XFS filesytem (I can't
remember any reports to the upstream list for any kernel) on a 3.2
kernel, then correct answer is "upgrade to a newer upstream kernel",
not "do a risky backport that exposes the entire user base to the
potential of new data corruption bugs"....

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

Re: XFS hole punch races

[Ben Hutchings]

[-- Attachment #1.1: Type: text/plain, Size: 2937 bytes --]

On Sun, 2016-06-05 at 09:28 +1000, Dave Chinner wrote:
> On Sat, Jun 04, 2016 at 06:11:10PM +0100, Ben Hutchings wrote:
> > On Tue, 2016-03-22 at 16:57 +0100, Jan Kara wrote:
> > 
> > Hi,
> > 
> > similarly to ext4 also XFS had races between hole punching and page faults
> > which could result in data corruption. The fixes were merged in 4.1-rc1 but
> > it might make sense to backport them to older stable releases given the
> > nature of the issue.
> > 
> > Relevant fixes are:
> > 
> > de0e8c20ba3a65b0f15040aabbefdc1999876e6b
> > 075a924d45cc69c75a35f20b4912b85aa98b180a
> > e8e9ad42c1f1e1bfbe0e8c32c8cac02e9ebfb7ef
> > 0f9160b444e4de33b65dfcd3b901358a3129461a
> > 723cac48473358939759885a18e8df113ea96138
> > ec56b1f1fdc69599963574ce94cc5693d535dd64
> > 
> > 
> > You missed the first in that sequence:
> > 
> > 653c60b633a9 xfs: introduce mmap/truncate lock
> > 
> > For 3.16, I've queued up all those fixes with one further prerequisite:
> > 
> > 812176832169 xfs: fix swapext ilock deadlock
> > 
> > For 3.2, I've queued up all but 723cac484733, with these additional
> > prerequisites:
> > 
> > f38996f57687 xfs: reduce ilock hold times in xfs_setattr_size
> > bc4010ecb8f4 xfs: use iolock on XFS_IOC_ALLOCSP calls
> > 76ca4c238cf5 xfs: always take the iolock around xfs_setattr_size
> > 5f8aca8b43f4 xfs: always hold the iolock when calling xfs_change_file_space
> > I realise I'll need to check for regressions with xfstests.
> 
> Hi Ben,
> 
> You do realise that this sort of backport effectively makes the
> stable kernels unsupportable by the upstream XFS developers? You're
> taking random changes from the upstream kernel until the kernel
> compiles, and then mostly hoping that it works.

I'm applying slightly more intelligence than that, but of course I'm
not an XFS developer.

> It's trivially easy to break truncate by screwing up the locking and
> IO barriers that it depends on, and this set of patches make quite a
> lot of different changes that have an unknown set of dependencies
> for correct behaviour. xfstests won't find all those problems; at
> best it will tell us that there won't be obvious problems.
> 
> Stable kernels are supposed to be stable, and backports like this
> are riskier than the original changes in the upstream kernels. if a
> user is hitting a mmap/hole-punch race on an XFS filesytem (I can't
> remember any reports to the upstream list for any kernel) on a 3.2
> kernel, then correct answer is "upgrade to a newer upstream kernel",
> not "do a risky backport that exposes the entire user base to the
> potential of new data corruption bugs"....

OK, then I'll drop them for 3.2.

Ben.

-- 
Ben Hutchings
Everything should be made as simple as possible, but not simpler.
                                                           - Albert
Einstein

[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

[-- Attachment #2: Type: text/plain, Size: 121 bytes --]

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: XFS hole punch races

[Ben Hutchings]

[-- Attachment #1: Type: text/plain, Size: 2937 bytes --]

On Sun, 2016-06-05 at 09:28 +1000, Dave Chinner wrote:
> On Sat, Jun 04, 2016 at 06:11:10PM +0100, Ben Hutchings wrote:
> > On Tue, 2016-03-22 at 16:57 +0100, Jan Kara wrote:
> > 
> > Hi,
> > 
> > similarly to ext4 also XFS had races between hole punching and page faults
> > which could result in data corruption. The fixes were merged in 4.1-rc1 but
> > it might make sense to backport them to older stable releases given the
> > nature of the issue.
> > 
> > Relevant fixes are:
> > 
> > de0e8c20ba3a65b0f15040aabbefdc1999876e6b
> > 075a924d45cc69c75a35f20b4912b85aa98b180a
> > e8e9ad42c1f1e1bfbe0e8c32c8cac02e9ebfb7ef
> > 0f9160b444e4de33b65dfcd3b901358a3129461a
> > 723cac48473358939759885a18e8df113ea96138
> > ec56b1f1fdc69599963574ce94cc5693d535dd64
> > 
> > 
> > You missed the first in that sequence:
> > 
> > 653c60b633a9 xfs: introduce mmap/truncate lock
> > 
> > For 3.16, I've queued up all those fixes with one further prerequisite:
> > 
> > 812176832169 xfs: fix swapext ilock deadlock
> > 
> > For 3.2, I've queued up all but 723cac484733, with these additional
> > prerequisites:
> > 
> > f38996f57687 xfs: reduce ilock hold times in xfs_setattr_size
> > bc4010ecb8f4 xfs: use iolock on XFS_IOC_ALLOCSP calls
> > 76ca4c238cf5 xfs: always take the iolock around xfs_setattr_size
> > 5f8aca8b43f4 xfs: always hold the iolock when calling xfs_change_file_space
> > I realise I'll need to check for regressions with xfstests.
> 
> Hi Ben,
> 
> You do realise that this sort of backport effectively makes the
> stable kernels unsupportable by the upstream XFS developers? You're
> taking random changes from the upstream kernel until the kernel
> compiles, and then mostly hoping that it works.

I'm applying slightly more intelligence than that, but of course I'm
not an XFS developer.

> It's trivially easy to break truncate by screwing up the locking and
> IO barriers that it depends on, and this set of patches make quite a
> lot of different changes that have an unknown set of dependencies
> for correct behaviour. xfstests won't find all those problems; at
> best it will tell us that there won't be obvious problems.
> 
> Stable kernels are supposed to be stable, and backports like this
> are riskier than the original changes in the upstream kernels. if a
> user is hitting a mmap/hole-punch race on an XFS filesytem (I can't
> remember any reports to the upstream list for any kernel) on a 3.2
> kernel, then correct answer is "upgrade to a newer upstream kernel",
> not "do a risky backport that exposes the entire user base to the
> potential of new data corruption bugs"....

OK, then I'll drop them for 3.2.

Ben.

-- 
Ben Hutchings
Everything should be made as simple as possible, but not simpler.
                                                           - Albert
Einstein

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: XFS hole punch races

[Dave Chinner]

On Sun, Jun 05, 2016 at 02:19:32AM +0100, Ben Hutchings wrote:
> On Sun, 2016-06-05 at 09:28 +1000, Dave Chinner wrote:
> > On Sat, Jun 04, 2016 at 06:11:10PM +0100, Ben Hutchings wrote:
> > > On Tue, 2016-03-22 at 16:57 +0100, Jan Kara wrote:
> > > 
> > > Hi,
> > > 
> > > similarly to ext4 also XFS had races between hole punching and page faults
> > > which could result in data corruption. The fixes were merged in 4.1-rc1 but
> > > it might make sense to backport them to older stable releases given the
> > > nature of the issue.
> > > 
> > > Relevant fixes are:
> > > 
> > > de0e8c20ba3a65b0f15040aabbefdc1999876e6b
> > > 075a924d45cc69c75a35f20b4912b85aa98b180a
> > > e8e9ad42c1f1e1bfbe0e8c32c8cac02e9ebfb7ef
> > > 0f9160b444e4de33b65dfcd3b901358a3129461a
> > > 723cac48473358939759885a18e8df113ea96138
> > > ec56b1f1fdc69599963574ce94cc5693d535dd64
> > > 
> > > 
> > > You missed the first in that sequence:
> > > 
> > > 653c60b633a9 xfs: introduce mmap/truncate lock
> > > 
> > > For 3.16, I've queued up all those fixes with one further prerequisite:
> > > 
> > > 812176832169 xfs: fix swapext ilock deadlock
> > > 
> > > For 3.2, I've queued up all but 723cac484733, with these additional
> > > prerequisites:
> > > 
> > > f38996f57687 xfs: reduce ilock hold times in xfs_setattr_size
> > > bc4010ecb8f4 xfs: use iolock on XFS_IOC_ALLOCSP calls
> > > 76ca4c238cf5 xfs: always take the iolock around xfs_setattr_size
> > > 5f8aca8b43f4 xfs: always hold the iolock when calling xfs_change_file_space
> > > I realise I'll need to check for regressions with xfstests.
> > 
> > Hi Ben,
> > 
> > You do realise that this sort of backport effectively makes the
> > stable kernels unsupportable by the upstream XFS developers? You're
> > taking random changes from the upstream kernel until the kernel
> > compiles, and then mostly hoping that it works.
> 
> I'm applying slightly more intelligence than that, but of course I'm
> not an XFS developer.

Sorry, Ben, I didn't mean to imply you hadn't done your due diligence
properly. It's more a case of lots of things around these patches
also changed, and from that perspective the changes are effective a
random selection of changes spread across several years of
development.

It's subtle things, like changes to how IO completion is processed
(especially for AIO), etc that the backported code might depend on
for correct behaviour but aren't in the older kernels. These sorts
of subtle problems are typically only discovered by users with
uncommon applications and/or load....

> > Stable kernels are supposed to be stable, and backports like this
> > are riskier than the original changes in the upstream kernels. if a
> > user is hitting a mmap/hole-punch race on an XFS filesytem (I can't
> > remember any reports to the upstream list for any kernel) on a 3.2
> > kernel, then correct answer is "upgrade to a newer upstream kernel",
> > not "do a risky backport that exposes the entire user base to the
> > potential of new data corruption bugs"....
> 
> OK, then I'll drop them for 3.2.

Probably for the best - these mmap/holepunch races had been in the
code since XFS was first ported, so it's not like it's a new
problem. :/

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: XFS hole punch races

[Dave Chinner]

On Sun, Jun 05, 2016 at 02:19:32AM +0100, Ben Hutchings wrote:
> On Sun, 2016-06-05 at 09:28 +1000, Dave Chinner wrote:
> > On Sat, Jun 04, 2016 at 06:11:10PM +0100, Ben Hutchings wrote:
> > > On Tue, 2016-03-22 at 16:57 +0100, Jan Kara wrote:
> > > 
> > > Hi,
> > > 
> > > similarly to ext4 also XFS had races between hole punching and page faults
> > > which could result in data corruption. The fixes were merged in 4.1-rc1 but
> > > it might make sense to backport them to older stable releases given the
> > > nature of the issue.
> > > 
> > > Relevant fixes are:
> > > 
> > > de0e8c20ba3a65b0f15040aabbefdc1999876e6b
> > > 075a924d45cc69c75a35f20b4912b85aa98b180a
> > > e8e9ad42c1f1e1bfbe0e8c32c8cac02e9ebfb7ef
> > > 0f9160b444e4de33b65dfcd3b901358a3129461a
> > > 723cac48473358939759885a18e8df113ea96138
> > > ec56b1f1fdc69599963574ce94cc5693d535dd64
> > > 
> > > 
> > > You missed the first in that sequence:
> > > 
> > > 653c60b633a9 xfs: introduce mmap/truncate lock
> > > 
> > > For 3.16, I've queued up all�those fixes with one further�prerequisite:
> > > 
> > > 812176832169 xfs: fix swapext ilock deadlock
> > > 
> > > For 3.2, I've queued up all but 723cac484733, with these additional
> > > prerequisites:
> > > 
> > > f38996f57687 xfs: reduce ilock hold times in xfs_setattr_size
> > > bc4010ecb8f4 xfs: use iolock on XFS_IOC_ALLOCSP calls
> > > 76ca4c238cf5 xfs: always take the iolock around xfs_setattr_size
> > > 5f8aca8b43f4 xfs: always hold the iolock when calling xfs_change_file_space
> > > I realise I'll need to check for regressions with xfstests.
> > 
> > Hi Ben,
> > 
> > You do realise that this sort of backport effectively makes the
> > stable kernels unsupportable by the upstream XFS developers? You're
> > taking random changes from the upstream kernel until the kernel
> > compiles, and then mostly hoping that it works.
> 
> I'm applying slightly more intelligence than that, but of course I'm
> not an XFS developer.

Sorry, Ben, I didn't mean to imply you hadn't done your due diligence
properly. It's more a case of lots of things around these patches
also changed, and from that perspective the changes are effective a
random selection of changes spread across several years of
development.

It's subtle things, like changes to how IO completion is processed
(especially for AIO), etc that the backported code might depend on
for correct behaviour but aren't in the older kernels. These sorts
of subtle problems are typically only discovered by users with
uncommon applications and/or load....

> > Stable kernels are supposed to be stable, and backports like this
> > are riskier than the original changes in the upstream kernels. if a
> > user is hitting a mmap/hole-punch race on an XFS filesytem (I can't
> > remember any reports to the upstream list for any kernel) on a 3.2
> > kernel, then correct answer is "upgrade to a newer upstream kernel",
> > not "do a risky backport that exposes the entire user base to the
> > potential of new data corruption bugs"....
> 
> OK, then I'll drop them for 3.2.

Probably for the best - these mmap/holepunch races had been in the
code since XFS was first ported, so it's not like it's a new
problem. :/

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

Re: XFS hole punch races

[Willy Tarreau]

Dave,

On Sun, Jun 05, 2016 at 12:16:54PM +1000, Dave Chinner wrote:
> On Sun, Jun 05, 2016 at 02:19:32AM +0100, Ben Hutchings wrote:
> > On Sun, 2016-06-05 at 09:28 +1000, Dave Chinner wrote:
> > > You do realise that this sort of backport effectively makes the
> > > stable kernels unsupportable by the upstream XFS developers? You're
> > > taking random changes from the upstream kernel until the kernel
> > > compiles, and then mostly hoping that it works.
> > 
> > I'm applying slightly more intelligence than that, but of course I'm
> > not an XFS developer.
> 
> Sorry, Ben, I didn't mean to imply you hadn't done your due diligence
> properly. It's more a case of lots of things around these patches
> also changed, and from that perspective the changes are effective a
> random selection of changes spread across several years of
> development.
> 
> It's subtle things, like changes to how IO completion is processed
> (especially for AIO), etc that the backported code might depend on
> for correct behaviour but aren't in the older kernels. These sorts
> of subtle problems are typically only discovered by users with
> uncommon applications and/or load....

Does this mean that as a rule of thumb we'd rather avoid backporting
XFS fixes unless they seem really obvious (or at all) ?

Thanks,
Willy

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: XFS hole punch races

[Willy Tarreau]

Dave,

On Sun, Jun 05, 2016 at 12:16:54PM +1000, Dave Chinner wrote:
> On Sun, Jun 05, 2016 at 02:19:32AM +0100, Ben Hutchings wrote:
> > On Sun, 2016-06-05 at 09:28 +1000, Dave Chinner wrote:
> > > You do realise that this sort of backport effectively makes the
> > > stable kernels unsupportable by the upstream XFS developers? You're
> > > taking random changes from the upstream kernel until the kernel
> > > compiles, and then mostly hoping that it works.
> > 
> > I'm applying slightly more intelligence than that, but of course I'm
> > not an XFS developer.
> 
> Sorry, Ben, I didn't mean to imply you hadn't done your due diligence
> properly. It's more a case of lots of things around these patches
> also changed, and from that perspective the changes are effective a
> random selection of changes spread across several years of
> development.
> 
> It's subtle things, like changes to how IO completion is processed
> (especially for AIO), etc that the backported code might depend on
> for correct behaviour but aren't in the older kernels. These sorts
> of subtle problems are typically only discovered by users with
> uncommon applications and/or load....

Does this mean that as a rule of thumb we'd rather avoid backporting
XFS fixes unless they seem really obvious (or at all) ?

Thanks,
Willy