current patch queue for 3.12

From: Jie Liu <[email protected]>

Validate log space during log mount stage, the underlying function will drop a warning message via syslog in critical level if the log space is too small or too large.

[ dchinner: For CRC enable filesystems, abort the mounting of the filesystem as mkfs should never make a log too small for the given filesystem configuration. ]

Signed-off-by Jie Liu <[email protected]>

Signed-off-by Dave Chinner <[email protected]>

---
 fs/xfs/xfs_log.c | 49 ++++++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 48 insertions(+), 1 deletion(-)

fs/xfs/xfs_log.c +8 -1

index b02415b..64b1f01 100644
@@ -614,7 +614,9 @@ xfs_log_mount(
 	xfs_daddr_t	blk_offset,
 	int		num_bblks)
 {
-	int		error;
+	int		error = 0;
+	int		min_logfsbs;
+	int		max_logfsbs;
 
 	if (!(mp->m_flags & XFS_MOUNT_NORECOVERY))
 		xfs_notice(mp, "Mounting Filesystem");

@@ -631,6 +633,51 @@ xfs_log_mount(
 	}
 
 	/*
+	 * Validate the given log space and drop a critical message via syslog
+	 * if the log size is too small that would lead to some unexpected
+	 * situations in transaction log space reservation stage.
+	 *
+	 * Note: we can't just reject the mount if the validation fails.  This

From: Dave Chinner<[email protected]>

To begin optimising the CIL commit process, we need to have IOP_SIZE return both the number of vectors and the size of the data pointed to by the vectors. This enables us to calculate the size ofthe memory allocation needed before the formatting step and reduces the number of memory allocations per item by one.

WhIle there, kill the IOP_SIZE macro.

Signed-off-by Dave Chinner<[email protected]>

From: Dave Chinner <[email protected]>

To begin optimising the CIL commit process, we need to have IOP_SIZE return both the number of vectors and the size of the data pointed to by the vectors. This enables us to calculate the size ofthe memory allocation needed before the formatting step and reduces the number of memory allocations per item by one.

WhIle there, kill the IOP_SIZE macro.

From: Dave Chinner<[email protected]>

Now that we have the size of the object before the formatting pass is called, we can allocation the log vector and it's buffer in a single allocation rather than two separate allocations.

Store the size of the allocated buffer in the log vector so that we potentially avoid allocation for future modifications of the object.

While touching this code, remove the IOP_FORMAT definition.

Signed-off-by Dave Chinner<[email protected]>

Quoted 6 lines · Dave Chinner

From: Dave Chinner<[email protected]>

Now that we have the size of the object before the formatting pass is called, we can allocation the log vector and it's buffer in a single allocation rather than two separate allocations.

Quoted 7 lines

Store the size of the allocated buffer in the log vector so that we potentially avoid allocation for future modifications of the object.

While touching this code, remove the IOP_FORMAT definition.

Signed-off-by Dave Chinner<[email protected]>

Looks good.

Reviewed-by Mark Tinguely <[email protected]>

_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs

Quoted 15 lines · Mark Tinguely

Quoted 6 lines · Dave Chinner

From: Dave Chinner<[email protected]>

Now that we have the size of the object before the formatting pass is called, we can allocation the log vector and it's buffer in a single allocation rather than two separate allocations.

Quoted 7 lines

Store the size of the allocated buffer in the log vector so that we potentially avoid allocation for future modifications of the object.

While touching this code, remove the IOP_FORMAT definition.

Signed-off-by Dave Chinner<[email protected]>

Quoted 8 lines

Looks good.

Reviewed-by Mark Tinguely<[email protected]>

_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs

I'd like to register a gentle "test this well" protest on this patch. While trying to figure out the origin of an unrelated lockdep, I tried to copy 3 kernel gits from one 2k non-CRC XFS filesystem to another one. With at least this patch used, the cp operatin stops, leading to not-umountable, not-syncable filesystems. It might be while copying the 2nd git, or the 3rd git, while copying header files, or while copying those large *.pack files, but it will happen somewhere.

A bisect of the issue ends on this patch, but its removal means that 45_49 and 46_49 cannot be applied without good knowledge of the code to be patched.

This one's on me for not being able to get good information to Dave. If I can find a way to get trace-cmd to pipe over ssh or something like that, then maybe there's a chance to make a file that trace-cmd report can read. Previous attempts to save to different filesystems or save over NFS and CIFS have all failed. Will keep trying...

For diagnosing this patch, is there an effective trace that is rather small? And would you need more than just the XFS events?

Thanks!

Michael

Quoted 50 lines · Michael L. Semon

Quoted 19 lines · Mark Tinguely

Quoted 7 lines · Dave Chinner

From: Dave Chinner<[email protected]>

Now that we have the size of the object before the formatting pass is called, we can allocation the log vector and it's buffer in a single allocation rather than two separate allocations.

Quoted 8 lines

Store the size of the allocated buffer in the log vector so that we potentially avoid allocation for future modifications of the object.

While touching this code, remove the IOP_FORMAT definition.

Signed-off-by Dave Chinner<[email protected]>

Quoted 8 lines

Looks good.

Reviewed-by Mark Tinguely<[email protected]>

_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs

I'd like to register a gentle "test this well" protest on this patch. While trying to figure out the origin of an unrelated lockdep, I tried to copy 3 kernel gits from one 2k non-CRC XFS filesystem to another one. With at least this patch used, the cp operatin stops, leading to not-umountable, not-syncable filesystems. It might be while copying the 2nd git, or the 3rd git, while copying header files, or while copying those large *.pack files, but it will happen somewhere.

A bisect of the issue ends on this patch, but its removal means that 45_49 and 46_49 cannot be applied without good knowledge of the code to be patched.

This one's on me for not being able to get good information to Dave. If I can find a way to get trace-cmd to pipe over ssh or something like that, then maybe there's a chance to make a file that trace-cmd report can read. Previous attempts to save to different filesystems or save over NFS and CIFS have all failed. Will keep trying...

Quoted 7 lines

For diagnosing this patch, is there an effective trace that is rather small? And would you need more than just the XFS events?

Thanks!

Michael

Thanks for the heads up.

If you could please redo the test and get the stack traces with /proc/sysrq-trigger and if you kernel works with crash, a core dump. For the stack trace, I mostly want to know if it has several "xlog_grant_head_wait" entries in it, because ...

...I seemed to have triggered a couple log space reservation hangs with fsstress one XFS partition and a mega-copy on another partition, but will have to graft the new XFS tree onto a Linux 3.10 kernel to get crash (and one of my sata controllers) to work again.

Thanks.

--Mark.

If you could please redo the test and get the stack traces with /proc/sysrq-trigger and if you kernel works with crash, a core dump. For the stack trace, I mostly want to know if it has several "xlog_grant_head_wait" entries in it, because ...

...I seemed to have triggered a couple log space reservation hangs with fsstress one XFS partition and a mega-copy on another partition, but will have to graft the new XFS tree onto a Linux 3.10 kernel to get crash (and one of my sata controllers) to work again.

Quoted 9 lines · Mark Tinguely

If you could please redo the test and get the stack traces with /proc/sysrq-trigger and if you kernel works with crash, a core dump. For the stack trace, I mostly want to know if it has several "xlog_grant_head_wait" entries in it, because ...

...I seemed to have triggered a couple log space reservation hangs with fsstress one XFS partition and a mega-copy on another partition, but will have to graft the new XFS tree onto a Linux 3.10 kernel to get crash (and one of my sata controllers) to work again.

They are unrelated to this patchset.

Somewhere in the code there
is a mismatch between what we reserve as the base requirement for an actual log write and what the CIL actually steals, and that is, most likely, what is leading to log hangs.

This is demonstratable in the fact that generic/070 on 512 byte block size filesystems regularly hits a transaction reservation exhausted assert failure on transaction commit of the periodic log dummy transaction on my test rigs.

Cheers,

Dave.

Quoted 18 lines · Dave Chinner

Quoted 9 lines · Mark Tinguely

If you could please redo the test and get the stack traces with /proc/sysrq-trigger and if you kernel works with crash, a core dump. For the stack trace, I mostly want to know if it has several "xlog_grant_head_wait" entries in it, because ...

...I seemed to have triggered a couple log space reservation hangs with fsstress one XFS partition and a mega-copy on another partition, but will have to graft the new XFS tree onto a Linux 3.10 kernel to get crash (and one of my sata controllers) to work again.

They are unrelated to this patchset.

Somewhere in the code there
is a mismatch between what we reserve as the base requirement for an actual log write and what the CIL actually steals, and that is, most likely, what is leading to log hangs.

Quoted 8 lines

This is demonstratable in the fact that generic/070 on 512 byte block size filesystems regularly hits a transaction reservation exhausted assert failure on transaction commit of the periodic log dummy transaction on my test rigs.

Cheers,

Dave.

In testing patch 44, I did not trip over any cil stealing asserts before the hang. I think the cil steal assert is a different and a legitimate complaint. When I tripped over the ASSERT in with the v3 inode enabled, the writeid only reserves space for the sb but there were occasions of root btree and attribute fork entry that were also logged.

patch 43 runs for hours without incident. Previous to this series, I ran the same tests with parent pointer testing with much higher log reservations for day or two and never got a hang.

I tested patch 44 with copy like tests and both times it hung both times - not a convincing number of tests. A quick look, I see an empty AIL, empty CIL, the CTX is using 0 bytes, doesn't look like there are any cil pushes going nor any older ctx, the ctx has an empty ticket reservation. The log tail is 0xd000014d7 and reserve/grant is 0xe00204d04. The next reservation is for a rename transaction that uses just over the log space left. There has to be a log space leak. I will go back patch 43 on one machine and patch 44 on another and make sure it is patch 44 is causing the problem.

Quoted 44 lines · Michael L. Semon

Quoted 15 lines · Mark Tinguely

Quoted 6 lines · Dave Chinner

From: Dave Chinner<[email protected]>

Now that we have the size of the object before the formatting pass is called, we can allocation the log vector and it's buffer in a single allocation rather than two separate allocations.

Quoted 7 lines

Store the size of the allocated buffer in the log vector so that we potentially avoid allocation for future modifications of the object.

While touching this code, remove the IOP_FORMAT definition.

Signed-off-by Dave Chinner<[email protected]>

Quoted 8 lines

Looks good.

Reviewed-by Mark Tinguely<[email protected]>

_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs

I'd like to register a gentle "test this well" protest on this patch. While trying to figure out the origin of an unrelated lockdep, I tried to copy 3 kernel gits from one 2k non-CRC XFS filesystem to another one. With at least this patch used, the cp operatin stops, leading to not-umountable, not-syncable filesystems. It might be while copying the 2nd git, or the 3rd git, while copying header files, or while copying those large *.pack files, but it will happen somewhere.

A bisect of the issue ends on this patch, but its removal means that 45_49 and 46_49 cannot be applied without good knowledge of the code to be patched.

This one's on me for not being able to get good information to Dave. If I can find a way to get trace-cmd to pipe over ssh or something like that, then maybe there's a chance to make a file that trace-cmd report can read. Previous attempts to save to different filesystems or save over NFS and CIFS have all failed. Will keep trying...

Quoted 7 lines

For diagnosing this patch, is there an effective trace that is rather small? And would you need more than just the XFS events?

Thanks!

Michael

Thanks for the heads up.

If you could please redo the test and get the stack traces with /proc/sysrq-trigger and if you kernel works with crash, a core dump. For the stack trace, I mostly want to know if it has several "xlog_grant_head_wait" entries in it, because ...

...I seemed to have triggered a couple log space reservation hangs with fsstress one XFS partition and a mega-copy on another partition, but will have to graft the new XFS tree onto a Linux 3.10 kernel to get crash (and one of my sata controllers) to work again.

Thanks.

--Mark.

Quoted 55 lines · Mark Tinguely

Quoted 44 lines · Michael L. Semon

Quoted 15 lines · Mark Tinguely

Quoted 6 lines · Dave Chinner

From: Dave Chinner<[email protected]>

Now that we have the size of the object before the formatting pass is called, we can allocation the log vector and it's buffer in a single allocation rather than two separate allocations.

Quoted 7 lines

Store the size of the allocated buffer in the log vector so that we potentially avoid allocation for future modifications of the object.

While touching this code, remove the IOP_FORMAT definition.

Signed-off-by Dave Chinner<[email protected]>

Quoted 8 lines

Looks good.

Reviewed-by Mark Tinguely<[email protected]>

_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs

I'd like to register a gentle "test this well" protest on this patch. While trying to figure out the origin of an unrelated lockdep, I tried to copy 3 kernel gits from one 2k non-CRC XFS filesystem to another one. With at least this patch used, the cp operatin stops, leading to not-umountable, not-syncable filesystems. It might be while copying the 2nd git, or the 3rd git, while copying header files, or while copying those large *.pack files, but it will happen somewhere.

A bisect of the issue ends on this patch, but its removal means that 45_49 and 46_49 cannot be applied without good knowledge of the code to be patched.

This one's on me for not being able to get good information to Dave. If I can find a way to get trace-cmd to pipe over ssh or something like that, then maybe there's a chance to make a file that trace-cmd report can read. Previous attempts to save to different filesystems or save over NFS and CIFS have all failed. Will keep trying...

Quoted 7 lines

For diagnosing this patch, is there an effective trace that is rather small? And would you need more than just the XFS events?

Thanks!

Michael

Thanks for the heads up.

Quoted 7 lines

If you could please redo the test and get the stack traces with /proc/sysrq-trigger and if you kernel works with crash, a core dump. For the stack trace, I mostly want to know if it has several "xlog_grant_head_wait" entries in it, because ...

...I seemed to have triggered a couple log space reservation hangs with fsstress one XFS partition and a mega-copy on another partition, but will have to graft the new XFS tree onto a Linux 3.10 kernel to get crash (and one of my sata controllers) to work again.

Thanks.

--Mark.

Well, I tried. Here's a Google Drive link, and hopefully it works:

https://drive.google.com/folderview?id=0B41268QKoNjtckwzVTJqWnIydEE

The instructions in Documentation/kdump/kdump.txt were followed fairly well. The dump was taken from /proc/vmcore and extracted like this:

$ cat /proc/vmcore | gzip -9vc>  4git-cp-kernel-dump-1.gz

You seem to have things well under control, so it all might not be needed, anyway. It does mean that kernel core dumps will go more quickly next time.

BTW, there's a "crash" program referenced in kdump.txt, and it's been downloaded but not built yet. Were you looking for output from the crash program?

Thanks!

Michael

Quoted 78 lines · Michael L. Semon

Quoted 55 lines · Mark Tinguely

Quoted 44 lines · Michael L. Semon

Quoted 15 lines · Mark Tinguely

Quoted 6 lines · Dave Chinner

From: Dave Chinner<[email protected]>

Now that we have the size of the object before the formatting pass is called, we can allocation the log vector and it's buffer in a single allocation rather than two separate allocations.

Quoted 7 lines

Store the size of the allocated buffer in the log vector so that we potentially avoid allocation for future modifications of the object.

While touching this code, remove the IOP_FORMAT definition.

Signed-off-by Dave Chinner<[email protected]>

Quoted 8 lines

Looks good.

Reviewed-by Mark Tinguely<[email protected]>

_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs

I'd like to register a gentle "test this well" protest on this patch. While trying to figure out the origin of an unrelated lockdep, I tried to copy 3 kernel gits from one 2k non-CRC XFS filesystem to another one. With at least this patch used, the cp operatin stops, leading to not-umountable, not-syncable filesystems. It might be while copying the 2nd git, or the 3rd git, while copying header files, or while copying those large *.pack files, but it will happen somewhere.

A bisect of the issue ends on this patch, but its removal means that 45_49 and 46_49 cannot be applied without good knowledge of the code to be patched.

This one's on me for not being able to get good information to Dave. If I can find a way to get trace-cmd to pipe over ssh or something like that, then maybe there's a chance to make a file that trace-cmd report can read. Previous attempts to save to different filesystems or save over NFS and CIFS have all failed. Will keep trying...

Quoted 7 lines

For diagnosing this patch, is there an effective trace that is rather small? And would you need more than just the XFS events?

Thanks!

Michael

Thanks for the heads up.

Quoted 7 lines

If you could please redo the test and get the stack traces with /proc/sysrq-trigger and if you kernel works with crash, a core dump. For the stack trace, I mostly want to know if it has several "xlog_grant_head_wait" entries in it, because ...

...I seemed to have triggered a couple log space reservation hangs with fsstress one XFS partition and a mega-copy on another partition, but will have to graft the new XFS tree onto a Linux 3.10 kernel to get crash (and one of my sata controllers) to work again.

Thanks.

--Mark.

Well, I tried. Here's a Google Drive link, and hopefully it works:

https://drive.google.com/folderview?id=0B41268QKoNjtckwzVTJqWnIydEE

The instructions in Documentation/kdump/kdump.txt were followed fairly well. The dump was taken from /proc/vmcore and extracted like this:

$ cat /proc/vmcore | gzip -9vc>  4git-cp-kernel-dump-1.gz

You seem to have things well under control, so it all might not be needed, anyway. It does mean that kernel core dumps will go more quickly next time.

Quoted 7 lines

BTW, there's a "crash" program referenced in kdump.txt, and it's been downloaded but not built yet. Were you looking for output from the crash program?

Thanks!

Michael

Thanks more data. I will take a look. I will need the vmlinux, System.map, and xfs.ko (if xfs is a module).

I can reproduce a problem in patch 44 too. It takes my copy test 20 minutes to deplete the log space with patch 44, Same test with patch 43 has been running a day and a half. I do not think that patch 44 is 100 times faster than patch 43 but I will let patch 43 spin all weekend on a couple machines to verify that patch 43 does not hang.

Whatever the cause, it has to be fixed before bringing in patches 43-47.

--Mark.

I can reproduce a problem in patch 44 too. It takes my copy test 20 minutes to deplete the log space with patch 44, Same test with patch 43 has been running a day and a half. I do not think that patch 44 is 100 times faster than patch 43 but I will let patch 43 spin all weekend on a couple machines to verify that patch 43 does not hang.

Quoted 6 lines · Mark Tinguely

I can reproduce a problem in patch 44 too. It takes my copy test 20 minutes to deplete the log space with patch 44, Same test with patch 43 has been running a day and a half. I do not think that patch 44 is 100 times faster than patch 43 but I will let patch 43 spin all weekend on a couple machines to verify that patch 43 does not hang.

Details, please. What's your "copy test"?

http://xfs.org/index.php/XFS_FAQ#Q:_What_information_should_I_include_when_reporting_a_problem.3F

Cheers,

Dave.

Quoted 8 lines · Dave Chinner

Quoted 6 lines · Mark Tinguely

I can reproduce a problem in patch 44 too. It takes my copy test 20 minutes to deplete the log space with patch 44, Same test with patch 43 has been running a day and a half. I do not think that patch 44 is 100 times faster than patch 43 but I will let patch 43 spin all weekend on a couple machines to verify that patch 43 does not hang.

Quoted 7 lines

Details, please. What's your "copy test"?

http://xfs.org/index.php/XFS_FAQ#Q:_What_information_should_I_include_when_reporting_a_problem.3F

Cheers,

Dave.

Micheal found the problem using a simple copy, so I am using copy-like test.

I have hung patch 44 with fsstress but I am currently using the simple perl scripts from bugzilla bug 922, but I do not do the silly step of reducing the log to an abnormal amount.

Just mkfs the filesystem

Create the the test files. the creation script:
http://oss.sgi.com/bugzilla/attachment.cgi?id=304

Run the test script:
http://oss.sgi.com/bugzilla/attachment.cgi?id=305

          ----

This test seems to stress the log much like xfstest 273 only it runs forever.

Quoted 25 lines · Mark Tinguely

Quoted 8 lines · Dave Chinner

Quoted 6 lines · Mark Tinguely

I can reproduce a problem in patch 44 too. It takes my copy test 20 minutes to deplete the log space with patch 44, Same test with patch 43 has been running a day and a half. I do not think that patch 44 is 100 times faster than patch 43 but I will let patch 43 spin all weekend on a couple machines to verify that patch 43 does not hang.

Quoted 7 lines

Details, please. What's your "copy test"?

http://xfs.org/index.php/XFS_FAQ#Q:_What_information_should_I_include_when_reporting_a_problem.3F

Cheers,

Dave.

Micheal found the problem using a simple copy, so I am using copy-like test.

I have hung patch 44 with fsstress but I am currently using the simple perl scripts from bugzilla bug 922, but I do not do the silly step of reducing the log to an abnormal amount.

Just mkfs the filesystem

Quoted 8 lines

Create the the test files. the creation script:

   http://oss.sgi.com/bugzilla/attachment.cgi?id=304

Run the test script:

   http://oss.sgi.com/bugzilla/attachment.cgi?id=305
           ----

This test seems to stress the log much like xfstest 273 only it runs forever.

Ok, thanks, for the info, Mark. I'll try to reproduce it here. FWIW, we should probably add this to xfstests as a "stress" test. i.e. not part of the auto group, but something that uses the scale parameters to determine runtime. then add all the other tests that have scale parameters to the same group, and if we run ./check --stress<scale> it runs through all the stress tests with that given scale factor...

Cheers,

Dave.

Quoted 11 lines · Dave Chinner

Quoted 10 lines · Mark Tinguely

Quoted 9 lines · Dave Chinner

Quoted 6 lines · Mark Tinguely

I can reproduce a problem in patch 44 too. It takes my copy test 20 minutes to deplete the log space with patch 44, Same test with patch 43 has been running a day and a half. I do not think that patch 44 is 100 times faster than patch 43 but I will let patch 43 spin all weekend on a couple machines to verify that patch 43 does not hang.

Details, please. What's your "copy test"?

Quoted 1 line

Quoted 1 line

Micheal found the problem using a simple copy, so I am using copy-like test.

BTW, the long term run of the copy.pl from bug 922 with patch 43 results:

 tail            0x601000055d7
 grant/reserve   0x60100abb200
 ctx t_unit_res  0x834

My log math may be off, tail/reserve diff is 1024, but the CTX holds more than that (2100 bytes).

Looking at patch 44, it is the first time we use the calculation for the number of bytes in patch 43. So I am looking at where the new calculation in iop_size differs from the previous len calculation in xlog_cil_prepare_log_vecs(). So far, I am that inode entry is 140 bytes larger with the new calculation (former len 164 vrs new nbytes 304 type 123b - non-crc filesystem).

I will see if I can substitute the nbytes for len (without triggering the ptr being exceeded assert) to see that will cause the hang.

Quoted 12 lines · Mark Tinguely

Quoted 11 lines · Dave Chinner

Quoted 10 lines · Mark Tinguely

Quoted 9 lines · Dave Chinner

Quoted 6 lines · Mark Tinguely

I can reproduce a problem in patch 44 too. It takes my copy test 20 minutes to deplete the log space with patch 44, Same test with patch 43 has been running a day and a half. I do not think that patch 44 is 100 times faster than patch 43 but I will let patch 43 spin all weekend on a couple machines to verify that patch 43 does not hang.

Details, please. What's your "copy test"?

....

Quoted 10 lines

Quoted 1 line

Quoted 1 line

Micheal found the problem using a simple copy, so I am using copy-like test.

BTW, the long term run of the copy.pl from bug 922 with patch 43 results:

  tail            0x601000055d7
  grant/reserve   0x60100abb200
  ctx t_unit_res  0x834

My log math may be off, tail/reserve diff is 1024, but the CTX holds more than that (2100 bytes).

Quoted 6 lines

Looking at patch 44, it is the first time we use the calculation for the number of bytes in patch 43. So I am looking at where the new calculation in iop_size differs from the previous len calculation in xlog_cil_prepare_log_vecs(). So far, I am that inode entry is 140 bytes larger with the new calculation (former len 164 vrs new nbytes 304 type 123b - non-crc filesystem).

Which size calculation is wrong? t~he one used to size the buffer being allocated - which is intentionally oversized for the inode forks - or the actual size formatted into the buffer, which was unchanged?

I mean, 164 bytes is an inode core (96 bytes) plus a inode log format structure. The increase of 140 bytes indicates that we are logging roughly the entire 256 byte inode - i.e. both forks.

But are you looking at the size returned by iop_size or iop_format? iop_size will be new, iop_format is unchanged by this patchset. indeed, what iop_format returns as vectors is *unchanged* by this patchset, so I think that you are chasing down the wrong path here.

Quoted 3 lines

I will see if I can substitute the nbytes for len (without triggering the ptr being exceeded assert) to see that will cause the hang.

I know what the problem is will be, and that won't fix it. Hint: the buffer is over sized.

Cheers,

Dave.

Quoted 40 lines · Dave Chinner

Quoted 12 lines · Mark Tinguely

Quoted 11 lines · Dave Chinner

Quoted 10 lines · Mark Tinguely

Quoted 9 lines · Dave Chinner

Quoted 6 lines · Mark Tinguely

I can reproduce a problem in patch 44 too. It takes my copy test 20 minutes to deplete the log space with patch 44, Same test with patch 43 has been running a day and a half. I do not think that patch 44 is 100 times faster than patch 43 but I will let patch 43 spin all weekend on a couple machines to verify that patch 43 does not hang.

Details, please. What's your "copy test"?

....

Quoted 10 lines

Quoted 1 line

Quoted 1 line

Micheal found the problem using a simple copy, so I am using copy-like test.

BTW, the long term run of the copy.pl from bug 922 with patch 43 results:

 tail            0x601000055d7
 grant/reserve   0x60100abb200
 ctx t_unit_res  0x834

My log math may be off, tail/reserve diff is 1024, but the CTX holds more than that (2100 bytes).

Quoted 6 lines

Looking at patch 44, it is the first time we use the calculation for the number of bytes in patch 43. So I am looking at where the new calculation in iop_size differs from the previous len calculation in xlog_cil_prepare_log_vecs(). So far, I am that inode entry is 140 bytes larger with the new calculation (former len 164 vrs new nbytes 304 type 123b - non-crc filesystem).

Which size calculation is wrong? t~he one used to size the buffer being allocated - which is intentionally oversized for the inode forks - or the actual size formatted into the buffer, which was unchanged?

I mean, 164 bytes is an inode core (96 bytes) plus a inode log format structure. The increase of 140 bytes indicates that we are logging roughly the entire 256 byte inode - i.e. both forks.

Quoted 5 lines

But are you looking at the size returned by iop_size or iop_format? iop_size will be new, iop_format is unchanged by this patchset. indeed, what iop_format returns as vectors is *unchanged* by this patchset, so I think that you are chasing down the wrong path here.

164 is the variable len. It is the sum of the xfs_log_iovec.i_len after format.

304 is the result of the iop_size(). starting in patch 44 that is used insteead of len. When we go do the format, we only use 164.

It is the XFS_DINODE_FMT_EXTENTS inode type that generates the extra 140 bytes in xfs_inode_item_size().

In patch 43, use nbytes instead of len like is done in patch 44 and it hangs at the same time and manner than patch 44.

From: Dave Chinner<[email protected]>

Now that we have the size of the log vector that has been allocated, we can determine if we need to allocate a new log vector for formatting and insertion. We only need to allocate a new vector if it won't fit into the existing buffer.

However, we need to hold the CIL context lock while we do this so that we can't race with a push draining the currently queued log vectors. It is safe to do this as long as we do GFP_NOFS allocation to avoid avoid memory allocation recursing into the filesystem. Hence we can safely overwrite the existing log vector on the CIL if it is large enough to hold all the dirty regions of the current item.

Signed-off-by Dave Chinner<[email protected]>

---

From: Dave Chinner<[email protected]>

Now that all the log item preparation and formatting is done under the CIL lock, we can get rid of the intermediate log vector chain used to track items to be inserted into the CIL.

We can already find all the items to be committed from the transaction handle, so as long as we attach the log vectors to the item before we insert the items into the CIL, we don't need to create a log vector chain to pass around.

This means we can move all the item insertion code into and optimise it into a pair of simple passes across all the items in the transaction. The first pass does the formatting and accounting, the second inserts them all into the CIL.

We keep this two pass split so that we can separate the CIL insertion - which must be done under the CIL spinlock - from the formatting. We could insert each item into the CIL with a single pass, but that massively increases the number of times we have to grab the CIL spinlock. It is much more efficient (and hence scalable) to do a batch operation and insert all objects in a single lock grab.

Signed-off-by Dave Chinner<[email protected]>

Quoted 25 lines · Dave Chinner

From: Dave Chinner<[email protected]>

Now that all the log item preparation and formatting is done under the CIL lock, we can get rid of the intermediate log vector chain used to track items to be inserted into the CIL.

We can already find all the items to be committed from the transaction handle, so as long as we attach the log vectors to the item before we insert the items into the CIL, we don't need to create a log vector chain to pass around.

This means we can move all the item insertion code into and optimise it into a pair of simple passes across all the items in the transaction. The first pass does the formatting and accounting, the second inserts them all into the CIL.

We keep this two pass split so that we can separate the CIL insertion - which must be done under the CIL spinlock - from the formatting. We could insert each item into the CIL with a single pass, but that massively increases the number of times we have to grab the CIL spinlock. It is much more efficient (and hence scalable) to do a batch operation and insert all objects in a single lock grab.

Signed-off-by Dave Chinner<[email protected]>

...

fs/xfs/xfs_log_cil.c xlog_cil_insert_items( · line 361

Quoted 6 lines

fs/xfs/xfs_log_cil.c +0 -2

@@ -357,10 +341,8 @@ xlog_cil_insert_items(
 	 * during the transaction commit.
 	 */
 	if (ctx->ticket->t_curr_res == 0) {
-		/* first commit in checkpoint, steal the header reservation */
-		ASSERT(ticket->t_curr_res>= ctx->ticket->t_unit_res + len);

^^ is the ctx ticket overflow caught somewhere else? ^^^^

From: Dave Chinner<[email protected]>

Now that all the log item preparation and formatting is done under the CIL lock, we can get rid of the intermediate log vector chain used to track items to be inserted into the CIL.

We can already find all the items to be committed from the transaction handle, so as long as we attach the log vectors to the item before we insert the items into the CIL, we don't need to create a log vector chain to pass around.

This means we can move all the item insertion code into and optimise it into a pair of simple passes across all the items in the transaction. The first pass does the formatting and accounting, the second inserts them all into the CIL.

We keep this two pass split so that we can separate the CIL insertion - which must be done under the CIL spinlock - from the formatting. We could insert each item into the CIL with a single pass, but that massively increases the number of times we have to grab the CIL spinlock. It is much more efficient (and hence scalable) to do a batch operation and insert all objects in a single lock grab.

Signed-off-by Dave Chinner<[email protected]>

---

From: Dave Chinner<[email protected]>

The xc_cil_lock is used for two purposes - to protect the CIL itself, and to protect the push/commit state and lists. These are two logically separate structures and operations, so can have their own locks. This means that pushing on the CIL and the commit wait ordering won't contend for a lock with other transactions that are completing concurrently. As the CIL insertion is the hottest path throught eh CIL, this is a big win.

Signed-off-by Dave Chinner<[email protected]>

---

From: Dave Chinner<[email protected]>

Add support for the file type field in directory entries so that readdir can return the type of the inode the dirent points to to userspace without first having to read the inode off disk.

The encoding of the type field is a single byte that is added to the end of the directory entry name length. For all intents and purposes, it appends a "hidden" byte to the name field which contains the type information. As the directory entry is already of dynamic size, helpers are already required to access and decode the direct entry structures.

Hence the relevent extraction and iteration helpers are updated to understand the hidden byte. Helpers for reading and writing the filetype field from the directory entries are also added. Only the read helpers are used by this patch. It also adds all the code necessary to read the type information out of the dirents on disk.

Further we add the superblock feature bit and helpers to indicate that we understand the on-disk format change. This is not a compatible change - existing kernels cannot read the new format successfully - so an incompatible feature flag is added. We don't yet allow filesystems to mount with this flag yet - that will be added once write support is added.

Finally, the code to take the type from the VFS, convert it to an XFS on-disk type and put it into the xfs_name structures passed around is added, but the directory code does not use this field yet. That will be in the next patch.

Signed-off-by Dave Chinner<[email protected]>

---

Quoted 34 lines · Dave Chinner

From: Dave Chinner<[email protected]>

Add support for the file type field in directory entries so that readdir can return the type of the inode the dirent points to to userspace without first having to read the inode off disk.

The encoding of the type field is a single byte that is added to the end of the directory entry name length. For all intents and purposes, it appends a "hidden" byte to the name field which contains the type information. As the directory entry is already of dynamic size, helpers are already required to access and decode the direct entry structures.

Hence the relevent extraction and iteration helpers are updated to understand the hidden byte. Helpers for reading and writing the filetype field from the directory entries are also added. Only the read helpers are used by this patch. It also adds all the code necessary to read the type information out of the dirents on disk.

Further we add the superblock feature bit and helpers to indicate that we understand the on-disk format change. This is not a compatible change - existing kernels cannot read the new format successfully - so an incompatible feature flag is added. We don't yet allow filesystems to mount with this flag yet - that will be added once write support is added.

Finally, the code to take the type from the VFS, convert it to an XFS on-disk type and put it into the xfs_name structures passed around is added, but the directory code does not use this field yet. That will be in the next patch.

Signed-off-by Dave Chinner<[email protected]>

---

fs/xfs/xfs_sb.h xfs_sb_has_incompat_log_feature( · line 630

Quoted 6 lines

fs/xfs/xfs_sb.h +4 -0

+static inline int xfs_sb_version_hasftype(struct xfs_sb *sbp)
+{
+	return XFS_SB_VERSION_NUM(sbp) == XFS_SB_VERSION_5&&
+		xfs_sb_has_incompat_feature(sbp, XFS_SB_FEAT_INCOMPAT_FTYPE);
 }

This feature should support inode version 2 and 3.

Quoted 36 lines · Mark Tinguely

Quoted 34 lines · Dave Chinner

From: Dave Chinner<[email protected]>

Add support for the file type field in directory entries so that readdir can return the type of the inode the dirent points to to userspace without first having to read the inode off disk.

The encoding of the type field is a single byte that is added to the end of the directory entry name length. For all intents and purposes, it appends a "hidden" byte to the name field which contains the type information. As the directory entry is already of dynamic size, helpers are already required to access and decode the direct entry structures.

Hence the relevent extraction and iteration helpers are updated to understand the hidden byte. Helpers for reading and writing the filetype field from the directory entries are also added. Only the read helpers are used by this patch. It also adds all the code necessary to read the type information out of the dirents on disk.

Further we add the superblock feature bit and helpers to indicate that we understand the on-disk format change. This is not a compatible change - existing kernels cannot read the new format successfully - so an incompatible feature flag is added. We don't yet allow filesystems to mount with this flag yet - that will be added once write support is added.

Finally, the code to take the type from the VFS, convert it to an XFS on-disk type and put it into the xfs_name structures passed around is added, but the directory code does not use this field yet. That will be in the next patch.

Signed-off-by Dave Chinner<[email protected]>

---

Quoted 8 lines

fs/xfs/xfs_sb.h xfs_sb_has_incompat_log_feature( · line 630

Quoted 6 lines

fs/xfs/xfs_sb.h +4 -0

+static inline int xfs_sb_version_hasftype(struct xfs_sb *sbp)
+{
+	return XFS_SB_VERSION_NUM(sbp) == XFS_SB_VERSION_5&&
+		xfs_sb_has_incompat_feature(sbp, XFS_SB_FEAT_INCOMPAT_FTYPE);
  }

This feature should support inode version 2 and 3.

Has nothing to do with the inode version number - it has to do with the directory structure being modified.

We're changing the directory structure for CRCs, and this builds on top of that. It is essentially part of the V3 directory format, and should be treated as such. Suggesting that we retrofit and support a modified v2 directory format is close to insane - instead of only having to test v2 vs v3 directory formats, you're suggesting we support v2 vs v2+dtype vs v3 vs v3+dtype. We simply do not have the resources to adequately test and support such an explosion of filesystem configurations.

We've had this discussion before - new on-disk features go into the v5 superblock format - the v4 superblock format from this point onwards is essentially legacy support from an upstream development perspective. Upstream development is about moving forward as quickly and cleanly as possible - quadrupling the test matrix for every minor on-disk change is simply not something we can sustain, and that's why I'm pushing back *hard* on any suggestion that we shoul dbe supporting new on-disk format changes on both v4 and v5 superblocks formats. v5 is the future and so all new features need to target v5 filesystem formats and ignore the restrictions that v4 filesystem formats might place on them.

That said, there's nothing to stop anyone from backporting such a feature to an older kernel and maintaining it themselves - it's open source software. But the idea that development should be constrained by having to support both old and new formats is wrong - the old v4 format should be considered stable and we need think very hard about changing it at all now, especially as much of the development focus is now shifting to taking advantage of the additions to the v5 format....

Cheers,

Dave.

Quoted 79 lines · Dave Chinner

Quoted 36 lines · Mark Tinguely

Quoted 34 lines · Dave Chinner

From: Dave Chinner<[email protected]>

Add support for the file type field in directory entries so that readdir can return the type of the inode the dirent points to to userspace without first having to read the inode off disk.

The encoding of the type field is a single byte that is added to the end of the directory entry name length. For all intents and purposes, it appends a "hidden" byte to the name field which contains the type information. As the directory entry is already of dynamic size, helpers are already required to access and decode the direct entry structures.

Hence the relevent extraction and iteration helpers are updated to understand the hidden byte. Helpers for reading and writing the filetype field from the directory entries are also added. Only the read helpers are used by this patch. It also adds all the code necessary to read the type information out of the dirents on disk.

Further we add the superblock feature bit and helpers to indicate that we understand the on-disk format change. This is not a compatible change - existing kernels cannot read the new format successfully - so an incompatible feature flag is added. We don't yet allow filesystems to mount with this flag yet - that will be added once write support is added.

Finally, the code to take the type from the VFS, convert it to an XFS on-disk type and put it into the xfs_name structures passed around is added, but the directory code does not use this field yet. That will be in the next patch.

Signed-off-by Dave Chinner<[email protected]>

---

Quoted 8 lines

fs/xfs/xfs_sb.h xfs_sb_has_incompat_log_feature( · line 630

Quoted 6 lines

fs/xfs/xfs_sb.h +4 -0

+static inline int xfs_sb_version_hasftype(struct xfs_sb *sbp)
+{
+	return XFS_SB_VERSION_NUM(sbp) == XFS_SB_VERSION_5&&
+		xfs_sb_has_incompat_feature(sbp, XFS_SB_FEAT_INCOMPAT_FTYPE);
 }

This feature should support inode version 2 and 3.

Has nothing to do with the inode version number - it has to do with the directory structure being modified.

We're changing the directory structure for CRCs, and this builds on top of that. It is essentially part of the V3 directory format, and should be treated as such. Suggesting that we retrofit and support a modified v2 directory format is close to insane - instead of only having to test v2 vs v3 directory formats, you're suggesting we support v2 vs v2+dtype vs v3 vs v3+dtype. We simply do not have the resources to adequately test and support such an explosion of filesystem configurations.

We've had this discussion before - new on-disk features go into the v5 superblock format - the v4 superblock format from this point onwards is essentially legacy support from an upstream development perspective. Upstream development is about moving forward as quickly and cleanly as possible - quadrupling the test matrix for every minor on-disk change is simply not something we can sustain, and that's why I'm pushing back *hard* on any suggestion that we shoul dbe supporting new on-disk format changes on both v4 and v5 superblocks formats. v5 is the future and so all new features need to target v5 filesystem formats and ignore the restrictions that v4 filesystem formats might place on them.

That said, there's nothing to stop anyone from backporting such a feature to an older kernel and maintaining it themselves - it's open source software. But the idea that development should be constrained by having to support both old and new formats is wrong - the old v4 format should be considered stable and we need think very hard about changing it at all now, especially as much of the development focus is now shifting to taking advantage of the additions to the v5 format....

Quoted 3 lines

Cheers,

Dave.

I guess we need more time to argue this out. It is not going into Linux 3.12 as a crc feature only.

Quoted 80 lines · Mark Tinguely

Quoted 79 lines · Dave Chinner

Quoted 36 lines · Mark Tinguely

Quoted 34 lines · Dave Chinner

From: Dave Chinner<[email protected]>

Add support for the file type field in directory entries so that readdir can return the type of the inode the dirent points to to userspace without first having to read the inode off disk.

The encoding of the type field is a single byte that is added to the end of the directory entry name length. For all intents and purposes, it appends a "hidden" byte to the name field which contains the type information. As the directory entry is already of dynamic size, helpers are already required to access and decode the direct entry structures.

Hence the relevent extraction and iteration helpers are updated to understand the hidden byte. Helpers for reading and writing the filetype field from the directory entries are also added. Only the read helpers are used by this patch. It also adds all the code necessary to read the type information out of the dirents on disk.

Further we add the superblock feature bit and helpers to indicate that we understand the on-disk format change. This is not a compatible change - existing kernels cannot read the new format successfully - so an incompatible feature flag is added. We don't yet allow filesystems to mount with this flag yet - that will be added once write support is added.

Finally, the code to take the type from the VFS, convert it to an XFS on-disk type and put it into the xfs_name structures passed around is added, but the directory code does not use this field yet. That will be in the next patch.

Signed-off-by Dave Chinner<[email protected]>

---

Quoted 8 lines

fs/xfs/xfs_sb.h xfs_sb_has_incompat_log_feature( · line 630

Quoted 6 lines

fs/xfs/xfs_sb.h +4 -0

+static inline int xfs_sb_version_hasftype(struct xfs_sb *sbp)
+{
+	return XFS_SB_VERSION_NUM(sbp) == XFS_SB_VERSION_5&&
+		xfs_sb_has_incompat_feature(sbp, XFS_SB_FEAT_INCOMPAT_FTYPE);
  }

This feature should support inode version 2 and 3.

Has nothing to do with the inode version number - it has to do with the directory structure being modified.

We're changing the directory structure for CRCs, and this builds on top of that. It is essentially part of the V3 directory format, and should be treated as such. Suggesting that we retrofit and support a modified v2 directory format is close to insane - instead of only having to test v2 vs v3 directory formats, you're suggesting we support v2 vs v2+dtype vs v3 vs v3+dtype. We simply do not have the resources to adequately test and support such an explosion of filesystem configurations.

We've had this discussion before - new on-disk features go into the v5 superblock format - the v4 superblock format from this point onwards is essentially legacy support from an upstream development perspective. Upstream development is about moving forward as quickly and cleanly as possible - quadrupling the test matrix for every minor on-disk change is simply not something we can sustain, and that's why I'm pushing back *hard* on any suggestion that we shoul dbe supporting new on-disk format changes on both v4 and v5 superblocks formats. v5 is the future and so all new features need to target v5 filesystem formats and ignore the restrictions that v4 filesystem formats might place on them.

That said, there's nothing to stop anyone from backporting such a feature to an older kernel and maintaining it themselves - it's open source software. But the idea that development should be constrained by having to support both old and new formats is wrong - the old v4 format should be considered stable and we need think very hard about changing it at all now, especially as much of the development focus is now shifting to taking advantage of the additions to the v5 format....

Quoted 6 lines

Quoted 3 lines

Cheers,

Dave.

I guess we need more time to argue this out. It is not going into Linux 3.12 as a crc feature only.

Seriously?

This was *always* intended as a feature that went along with the v3 directory modifications. For example, see slide 40 of this presentation:

http://xfs.org/images/d/d1/Xfs-scalability-lca2012.pdf

the d_type field is listed alongside version counters, inode create times, and unlinked list logging via inode items....

The only reason this has been done as a feature bit is because I didn't have it done in time to included it under the v5 superblock version change.

The directory format for v4 filesystems is not going to change. We do not have the resources to test it adequately, nor do we have the resources to support it adequately. We're already taking on a huge amount of risk with the v5 superblock changes, and our fallback position is that v4 superblock formats are unchanged and so unaffected by the format changes made for v5. Pushing format changes intended only for v5 fielsystems back into v4 filesystems jeopodises that fallback position and exposes all existing filesystems to increased risk.

And that is a risk I'm not willing to take on behalf of upstream kernel users. Those that move to CRC enabled filesystems know the risks of being early adopters and accept that there may be problems encountered. THose that are using v4 filesystems expect that there are no risky, destabilising changes made to the on-disk format, yet that's exactly what you are suggesting we do.

I'll repeat again what I've said all along - new on-disk format changes for upstream kernels need to be done on v5 filesystems as the v4 format is now considered stable and in maintenance mode. It takes months to to test and stabilise format changes and such changes do not align to the goals of maintaining a stable code base.

This is the same situationas when SGI changed from v3 superblocks to v4 superblocks back in the Irix 6.1/6.2 days. All new features targetted v4 superblocks, as the release process would result in all platforms shipping with v4 superblocks for all new filesystems with the next release a year later (6.5).

This is exactly the same situation - upstream development is solely focussed on future releases, and as such needs to make decisions based on what is best for us in the medium to long term. The v4 superblocks have lasted for 15 years, but we are now at a similar line in the sand - we have a major format and focus shift with v5 superblocks and that's where we need to be focussing our development direction and decisions.

Looking backwards by requiring new features to be chained to the old format is a bad direction to be taking - it compromises what we can do in the long term based on those features, and in the short term requires more resources to develop and test the new features. It's simply the wrong direction to be taking for upstream development.

This is why I'm pushing back hard on any suggestions that we target new features on v4 superblocks - it's a development dead end and therefore essentially a waste of upstream development resources....

v4 is hardly dead end. Feature bits can keep the filesystem stable.

v5 superblock is experimental and is not the automatically the default and only version.

Geoffrey has been expressing concerns about v5 and I agree with them. We came to the party too late, and despite our concerns, SGI has worked hard to get the crc pieces reviewed, tested and committed. The concerns are still there.

Yes, I am serious. v4 is not dead and should get new features where appropriate.

Quoted 9 lines · Dave Chinner

Quoted 8 lines · Mark Tinguely

Quoted 7 lines · Dave Chinner

Quoted 6 lines · Mark Tinguely

Quoted 5 lines · Dave Chinner

From: Dave Chinner<[email protected]>

Add support for the file type field in directory entries so that readdir can return the type of the inode the dirent points to to userspace without first having to read the inode off disk.

Quoted 29 lines

Quoted 29 lines

Quoted 29 lines

Quoted 4 lines

Quoted 3 lines

Signed-off-by Dave Chinner<[email protected]>

---

Quoted 8 lines

fs/xfs/xfs_sb.h xfs_sb_has_incompat_log_feature( · line 630

Quoted 6 lines

fs/xfs/xfs_sb.h +4 -0

+static inline int xfs_sb_version_hasftype(struct xfs_sb *sbp)
+{
+	return XFS_SB_VERSION_NUM(sbp) == XFS_SB_VERSION_5&&
+		xfs_sb_has_incompat_feature(sbp, XFS_SB_FEAT_INCOMPAT_FTYPE);
 }

This feature should support inode version 2 and 3.

Has nothing to do with the inode version number - it has to do with the directory structure being modified.

We're changing the directory structure for CRCs, and this builds on top of that. It is essentially part of the V3 directory format, and should be treated as such. Suggesting that we retrofit and support a modified v2 directory format is close to insane - instead of only having to test v2 vs v3 directory formats, you're suggesting we support v2 vs v2+dtype vs v3 vs v3+dtype. We simply do not have the resources to adequately test and support such an explosion of filesystem configurations.

We've had this discussion before - new on-disk features go into the v5 superblock format - the v4 superblock format from this point onwards is essentially legacy support from an upstream development perspective.

Quoted 9 lines

Quoted 9 lines

Quoted 8 lines

That said, there's nothing to stop anyone from backporting such a feature to an older kernel and maintaining it themselves - it's open source software. But the idea that development should be constrained by having to support both old and new formats is wrong - the old v4 format should be considered stable and we need think very hard about changing it at all now, especially as much of the development focus is now shifting to taking advantage of the additions to the v5 format....

Quoted 4 lines

Quoted 2 lines

I guess we need more time to argue this out. It is not going into Linux 3.12 as a crc feature only.

Seriously?

yes seriously.

Quoted 29 lines

This was *always* intended as a feature that went along with the v3 directory modifications. For example, see slide 40 of this presentation:

http://xfs.org/images/d/d1/Xfs-scalability-lca2012.pdf

the d_type field is listed alongside version counters, inode create times, and unlinked list logging via inode items....

The only reason this has been done as a feature bit is because I didn't have it done in time to included it under the v5 superblock version change.

The directory format for v4 filesystems is not going to change. We do not have the resources to test it adequately, nor do we have the resources to support it adequately. We're already taking on a huge amount of risk with the v5 superblock changes, and our fallback position is that v4 superblock formats are unchanged and so unaffected by the format changes made for v5. Pushing format changes intended only for v5 fielsystems back into v4 filesystems jeopodises that fallback position and exposes all existing filesystems to increased risk.

And that is a risk I'm not willing to take on behalf of upstream kernel users. Those that move to CRC enabled filesystems know the risks of being early adopters and accept that there may be problems encountered. THose that are using v4 filesystems expect that there are no risky, destabilising changes made to the on-disk format, yet that's exactly what you are suggesting we do.

Quoted 3 lines

This is why I'm pushing back hard on any suggestions that we target new features on v4 superblocks - it's a development dead end and therefore essentially a waste of upstream development resources....

v4 is hardly dead end. Feature bits can keep the filesystem stable.

v5 superblock is experimental and is not the automatically the default and only version.

Geoffrey has been expressing concerns about v5 and I agree with them. We came to the party too late, and despite our concerns, SGI has worked hard to get the crc pieces reviewed, tested and committed. The concerns are still there.

Yes, I am serious. v4 is not dead and should get new features where appropriate.

On Wed, Aug 14, 2013 at 05:50:42PM +1000, Dave Chinner wrote:

| On Tue, Aug 13, 2013 at 10:42:32AM -0500, Mark Tinguely wrote:
| > On 08/12/13 19:50, Dave Chinner wrote:
| > >On Mon, Aug 12, 2013 at 08:25:23AM -0500, Mark Tinguely wrote:
| > >>On 08/11/13 19:59, Dave Chinner wrote:
| > >>>On Tue, Jul 30, 2013 at 02:10:32PM -0500, Mark Tinguely wrote:
| > >>>>On 07/19/13 01:25, Dave Chinner wrote:
| > >>>>>From: Dave Chinner<[email protected]>
| > >>>>>
| > >>>>>Add support for the file type field in directory entries so that
| > >>>>>readdir can return the type of the inode the dirent points to to
| > >>>>>userspace without first having to read the inode off disk.
| .....
| > >>>>>Signed-off-by: Dave Chinner<[email protected]>
| > >>>>>---
| > >>>>>
| > >>>>
| > >>>>>+static inline int xfs_sb_version_hasftype(struct xfs_sb *sbp)
| > >>>>>+{
| > >>>>>+	return XFS_SB_VERSION_NUM(sbp) == XFS_SB_VERSION_5&&
| > >>>>>+		xfs_sb_has_incompat_feature(sbp, XFS_SB_FEAT_INCOMPAT_FTYPE);
| > >>>>>  }
| > >>>>>
| > >>>>
| > >>>>This feature should support inode version 2 and 3.
| > >>>
| > >>>Has nothing to do with the inode version number - it has to do with
| > >>>the directory structure being modified.
| > >>>
| > >>>We're changing the directory structure for CRCs, and this builds on
| > >>>top of that. It is essentially part of the V3 directory format, and
| > >>>should be treated as such. Suggesting that we retrofit and support a
| > >>>modified v2 directory format is close to insane - instead of only
| > >>>having to test v2 vs v3 directory formats, you're suggesting we
| > >>>support v2 vs v2+dtype vs v3 vs v3+dtype. We simply do not have the
| > >>>resources to adequately test and support such an explosion of
| > >>>filesystem configurations.
| > >>>
| > >>>We've had this discussion before - new on-disk features go into the
| > >>>v5 superblock format - the v4 superblock format from this point
| > >>>onwards is essentially legacy support from an upstream development
| > >>>perspective.
| ....
| > >>>That said, there's nothing to stop anyone from backporting such a
| > >>>feature to an older kernel and maintaining it themselves - it's open
| > >>>source software. But the idea that development should be constrained
| > >>>by having to support both old and new formats is wrong - the old v4
| > >>>format should be considered stable and we need think very hard about
| > >>>changing it at all now, especially as much of the development focus
| > >>>is now shifting to taking advantage of the additions to the v5
| > >>>format....
| > >>
| > >>I guess we need more time to argue this out. It is not going into
| > >>Linux 3.12 as a crc feature only.
| > >
| > >Seriously?
| > 
| > yes seriously.
| 
| Great, another random roadblock from the XFS maintainers to deal
| with.

The addition of the file type field to directory entries is a great new feature. Your implementation of adding a "hidden" byte to the name field is especially clever.

This is a feature that can benefit both
dir2 and dir3 format filesystems and is completely independent of your CRC and self describing metadata feature work. I understand that you are not interested in porting the capability to dir2 format filesystems yourself and do not have the resources to provide the associated testing and support.

Myself and others within SGI have discussed these issues,

and we are willing to take on the work ourselves rather than have this feature go only into v5 superblock filesystems where the feature is only accessible to those who are willing to risk using experimental code.

Given that SGI will be doing the work to support the file type field in dir2 format filesystems, it doesn't make sense to add the code to v5 filesystems until all of the work is complete as there could be additional considerations for the on disk changes.

We also noted that this feature should not be added to the kernel until userspace code is available to support this feature. Specifically, xfs_repair needs to validate and if needed repair the the file type field.

Also, tests are needed to validate the new functionality.

On Wed, Aug 14, 2013 at 05:50:42PM +1000, Dave Chinner wrote:

| On Tue, Aug 13, 2013 at 10:42:32AM -0500, Mark Tinguely wrote:
| > On 08/12/13 19:50, Dave Chinner wrote:
| > >On Mon, Aug 12, 2013 at 08:25:23AM -0500, Mark Tinguely wrote:
| > >>On 08/11/13 19:59, Dave Chinner wrote:
| > >>>On Tue, Jul 30, 2013 at 02:10:32PM -0500, Mark Tinguely wrote:
| > >>>>On 07/19/13 01:25, Dave Chinner wrote:
| > >>>>>From: Dave Chinner<[email protected]>
| > >>>>>
| > >>>>>Add support for the file type field in directory entries so that
| > >>>>>readdir can return the type of the inode the dirent points to to
| > >>>>>userspace without first having to read the inode off disk.
| .....
| > >>>>>Signed-off-by: Dave Chinner<[email protected]>
| > >>>>>---
| > >>>>>
| > >>>>
| > >>>>>+static inline int xfs_sb_version_hasftype(struct xfs_sb *sbp)
| > >>>>>+{
| > >>>>>+	return XFS_SB_VERSION_NUM(sbp) == XFS_SB_VERSION_5&&
| > >>>>>+		xfs_sb_has_incompat_feature(sbp, XFS_SB_FEAT_INCOMPAT_FTYPE);
| > >>>>>  }
| > >>>>>
| > >>>>
| > >>>>This feature should support inode version 2 and 3.
| > >>>
| > >>>Has nothing to do with the inode version number - it has to do with
| > >>>the directory structure being modified.
| > >>>
| > >>>We're changing the directory structure for CRCs, and this builds on
| > >>>top of that. It is essentially part of the V3 directory format, and
| > >>>should be treated as such. Suggesting that we retrofit and support a
| > >>>modified v2 directory format is close to insane - instead of only
| > >>>having to test v2 vs v3 directory formats, you're suggesting we
| > >>>support v2 vs v2+dtype vs v3 vs v3+dtype. We simply do not have the
| > >>>resources to adequately test and support such an explosion of
| > >>>filesystem configurations.
| > >>>
| > >>>We've had this discussion before - new on-disk features go into the
| > >>>v5 superblock format - the v4 superblock format from this point
| > >>>onwards is essentially legacy support from an upstream development
| > >>>perspective.
| ....
| > >>>That said, there's nothing to stop anyone from backporting such a
| > >>>feature to an older kernel and maintaining it themselves - it's open
| > >>>source software. But the idea that development should be constrained
| > >>>by having to support both old and new formats is wrong - the old v4
| > >>>format should be considered stable and we need think very hard about
| > >>>changing it at all now, especially as much of the development focus
| > >>>is now shifting to taking advantage of the additions to the v5
| > >>>format....
| > >>
| > >>I guess we need more time to argue this out. It is not going into
| > >>Linux 3.12 as a crc feature only.
| > >
| > >Seriously?
| >
| > yes seriously.
|
| Great, another random roadblock from the XFS maintainers to deal
| with.

The addition of the file type field to directory entries is a great new feature. Your implementation of adding a "hidden" byte to the name field is especially clever. This is a feature that can benefit both dir2 and dir3 format filesystems and is completely independent of your CRC and self describing metadata feature work. I understand that you are not interested in porting the capability to dir2 format filesystems yourself and do not have the resources to provide the associated testing and support. Myself and others within SGI have discussed these issues, and we are willing to take on the work ourselves rather than have this feature go only into v5 superblock filesystems where the feature is only accessible to those who are willing to risk using experimental code. Given that SGI will be doing the work to support the file type field in dir2 format filesystems, it doesn't make sense to add the code to v5 filesystems until all of the work is complete as there could be additional considerations for the on disk changes.

We also noted that this feature should not be added to the kernel until userspace code is available to support this feature. Specifically, xfs_repair needs to validate and if needed repair the the file type field. Also, tests are needed to validate the new functionality. While I expect that you will provide this support for v5 superblock and dir3 filesystems, one of us at SGI will extend the support to v4 superblock and dir2 filesystems.

On Thu, Aug 15, 2013 at 06:59:21AM +0100, Ric Wheeler wrote:

|
| As much as the community admires your brave willingness to protect us from
| code that was entirely developed by Dave, that is not really how the
| upstream kernel works.
|
| Dave is pretty much without equal in moving XFS along these days and this
| is a key feature that we are depending on.
|
| Please don't try to create hurdles to other people getting work into
| upstream, that kind of thing will lead to a fork of the XFS code base and
| ultimately that will be harder for all of us.
|
| It sounds like what you really should look at doing is to work inside of
| SGI to create a private, internal branch of the upstream code that you
| deliver in your product. Upstream code is all about innovation and new
| features, we don't let vendor specific, non-upstream branches become the
| place for hardening our code.

I agree Dave is a gifted, dedicated, passionate and prolific contributor to the XFS community. His contributions are greatly appreciated as are the contributions of all other members of the XFS community. Dave does stand out as the most significant code contributor to the project. He has played a significant role moving XFS forward both from within SGI and from outside SGI. This does not place his contributions beyond review or discussion however.

My objection to the current readdir filetype field feature code is that the feature is not being made available to mainstream XFS users, only to those willing to run experimental code. Dave has clearly stated that he will not do the back-porting work. I am not expecting Dave to do the back-porting work. Mark has volunteered to fill the back-porting role. Th porting work cannot be completed as the feature is not yet complete. The associated user space code changes have not been submitted, and tests to validate the new feature have not been submitted. I do not consider this to be an unreasonable request, even if there were back-porting required.

If Red Hat is depending on the readdir filetype field feature from Dave and do not wish to wait for the feature to be completed before pulling in the code, then perhaps Red Hat should create a internal branch of the upstream code that you can deliver in your products. The upstream code should be stable and fully functional, not a place for incomplete, partially tested, half finished work.

On Thu, Aug 15, 2013 at 10:41:47PM +0100, Ric Wheeler wrote:

| On 08/15/2013 07:04 PM, Geoffrey Wehrman wrote:
| >On Thu, Aug 15, 2013 at 06:59:21AM +0100, Ric Wheeler wrote:
| >|
| >| As much as the community admires your brave willingness to protect us 
| >from
| >| code that was entirely developed by Dave, that is not really how the
| >| upstream kernel works.
| >|
| >| Dave is pretty much without equal in moving XFS along these days and this
| >| is a key feature that we are depending on.
| >|
| >| Please don't try to create hurdles to other people getting work into
| >| upstream, that kind of thing will lead to a fork of the XFS code base and
| >| ultimately that will be harder for all of us.
| >|
| >| It sounds like what you really should look at doing is to work inside of
| >| SGI to create a private, internal branch of the upstream code that you
| >| deliver in your product. Upstream code is all about innovation and new
| >| features, we don't let vendor specific, non-upstream branches become the
| >| place for hardening our code.
| >
| >I agree Dave is a gifted, dedicated, passionate and prolific contributor
| >to the XFS community.  His contributions are greatly appreciated as are
| >the contributions of all other members of the XFS community.  Dave does
| >stand out as the most significant code contributor to the project.  He
| >has played a significant role moving XFS forward both from within SGI
| >and from outside SGI.  This does not place his contributions beyond
| >review or discussion however.
| >
| >My objection to the current readdir filetype field feature code is that
| >the feature is not being made available to mainstream XFS users, only
| >to those willing to run experimental code.  Dave has clearly stated that
| >he will not do the back-porting work.  I am not expecting Dave to do the
| >back-porting work.  Mark has volunteered to fill the back-porting role.
| >Th porting work cannot be completed as the feature is not yet complete.
| >The associated user space code changes have not been submitted,
| >and tests to validate the new feature have not been submitted.  I do
| >not consider this to be an unreasonable request, even if there were
| >back-porting required.
| >
| >If Red Hat is depending on the readdir filetype field feature from Dave
| >and do not wish to wait for the feature to be completed before pulling
| >in the code, then perhaps Red Hat should create a internal branch of
| >the upstream code that you can deliver in your products.  The upstream
| >code should be stable and fully functional, not a place for incomplete,
| >partially tested, half finished work.
| >
| >
| 
| I think that you are still unclear on what "mainstream" users are.
| 
| People who pay for commercial support use vendor branches of the kernel 
| (from us, from SLES, from you, etc).
| 
| Upstream users and community distros normally expect to see new features in 
| their kernels.
| 
| For what it's worth, Red Hat has been the largest contributor to XFS for 
| quite a while now & XFS will be our RHEL7 default file system.
| 
| We don't ship "untested" or "incomplete" code.

Clearly we all have the interests of Linux users in mind whether they be those paying for commercial support, developers embedding Linux in other products, or technically savvy end users. Clearly we don't agree all the time on content, time lines, and process. We each have to be willing to engage in a civil conversation and be willing to compromise.

I recognize that I tend to be a silent bystander until there is an issue where I disagree, and only then do I join the conversation. I do not get involved in the general conversations on the list. I have all kinds of excuses, but it comes down to the fact that I just don't make it a high enough priority. Likewise, I don't tell Dave, Christoph, Eric, and others how much I appreciate their contributions when I do not have an issue. That makes me nothing more than a whiner and complainer, and I apologize for as much.

I think it is great that Red Hat has been the largest contributor of XFS code recently. I'm excited that XFS will be the default filesystem for RHEL 7. I am not trying to sabotage any of Red Hat's plans. Success for Red Hat and XFS translates into success for all Linux users.

I would like to take this opportunity to point out that SGI's attempts to contribute code to XFS are frequently blocked by Red Hat without technical merit. Most recently we tried to submit code for the agskip mount option which SGI has been shipping for years. [http://oss.sgi.com/archives/xfs/2013-01/msg00561.html]

We asked for fields to be reserved in the new v3 inodes for parent pointers and allocation policies. That request was soundly rejected despite the existence of unreserved padding in the new inode format.
[http://oss.sgi.com/archives/xfs/2013-04/msg00214.html]

The response
to code submitted by SGI has been so negative, we don't even bother submitting most of our code to the list anymore as long as it does not affect the on-disk format.

A reiterate my appreciation of Red Hat's contributions to XFS. However, I hope that you and others at Red Hat recognize that Red Hat is not the sole source for innovation and contributions to XFS. The playing field must be kept level and everyone in the community must be allowed to participate.

On Mon, Aug 19, 2013 at 03:28:13PM +1000, Dave Chinner wrote:

| Participation requires being open about what you are doing. Posting
| designs and ideas for comment, communicating your vision for the
| future, posting code early and often, etc. That enables others to
| participate at every stage and means that problems/issues are caught
| early and rejection/acceptance occurs long before final code is
| written. That's the way open source software is developed and most
| of the XFS developer community understand that.

This is a statement we can all agree on.

| Except, it appears, the maintainers.

Don't blame the maintainer. I am not the maintainer. Send your insults my way, not to the maintainer.

The problem is that the maintainer (Ben) listens to all interested parties and does not take direction exclusively from you, Red Hat, SuSE, SGI, IBM, Oracle, or anyone else. He is caught in the middle. Let's be open here. The current conflict is the result of me insisting that your plan and vision on a specific feature is not in the best interest of the community and you do not like the direction I have presented. It is great that you have a vision, but not everyone agrees with your vision. You are definitely the most significant current contributor of code to XFS. Of your recent patch set, 49 of 50 patches were accepted. And the one patch that was not accepted, I asked for you to rework. You refused.

Mark agreed to rework your patch, and because of this Ben is accused of making conflicting decisions based on how each change impacts SGI's internal products, not what is in the best interests of the XFS community. Last time I checked, SGI was an active member of the XFS community even by your definition. Even so, I believe that the change I am requesting will benefit the entire XFS community, not just SGI. The problem I see here is that you are unwilling to compromise on your vision or on your interpretation of what is best for the community.

I would like to take this opportunity to point out that SGI's attempts to contribute code to XFS are frequently blocked by Red Hat without technical merit. Most recently we tried to submit code for the agskip mount option which SGI has been shipping for years. [http://oss.sgi.com/archives/xfs/2013-01/msg00561.html]

We asked for fields to be reserved in the new v3 inodes for parent pointers and allocation policies. That request was soundly rejected despite the existence of unreserved padding in the new inode format.
[http://oss.sgi.com/archives/xfs/2013-04/msg00214.html] The response to code submitted by SGI has been so negative

, we don't even bother
submitting most of our code to the list anymore as long as it does not affect the on-disk format.

The list of features and capabilities that we
carry in our internally maintained source trees is significant and long:
DMAPI, behavior chains, agskip mount option, ibound mount option, etc. These are all features that have been rejected by the external community but are of value to SGI customers.

A reiterate my appreciation of Red Hat's contributions to XFS. However, I hope that you and others at Red Hat recognize that Red Hat is not the sole source for innovation and contributions to XFS. The playing field must be kept level and everyone in the community must be allowed to participate.

On Wed, Aug 14, 2013 at 05:50:42PM +1000, Dave Chinner wrote:

| On Tue, Aug 13, 2013 at 10:42:32AM -0500, Mark Tinguely wrote:
| > On 08/12/13 19:50, Dave Chinner wrote:
| > >On Mon, Aug 12, 2013 at 08:25:23AM -0500, Mark Tinguely wrote:
| > >>On 08/11/13 19:59, Dave Chinner wrote:
| > >>>On Tue, Jul 30, 2013 at 02:10:32PM -0500, Mark Tinguely wrote:
| > >>>>On 07/19/13 01:25, Dave Chinner wrote:
| > >>>>>From: Dave Chinner<[email protected]>
| > >>>>>
| > >>>>>Add support for the file type field in directory entries so that
| > >>>>>readdir can return the type of the inode the dirent points to to
| > >>>>>userspace without first having to read the inode off disk.
| .....
| > >>>>>Signed-off-by: Dave Chinner<[email protected]>
| > >>>>>---
| > >>>>>
| > >>>>
| > >>>>>+static inline int xfs_sb_version_hasftype(struct xfs_sb *sbp)
| > >>>>>+{
| > >>>>>+	return XFS_SB_VERSION_NUM(sbp) == XFS_SB_VERSION_5&&
| > >>>>>+		xfs_sb_has_incompat_feature(sbp, XFS_SB_FEAT_INCOMPAT_FTYPE);
| > >>>>>  }
| > >>>>>
| > >>>>
| > >>>>This feature should support inode version 2 and 3.
| > >>>
| > >>>Has nothing to do with the inode version number - it has to do with
| > >>>the directory structure being modified.
| > >>>
| > >>>We're changing the directory structure for CRCs, and this builds on
| > >>>top of that. It is essentially part of the V3 directory format, and
| > >>>should be treated as such. Suggesting that we retrofit and support a
| > >>>modified v2 directory format is close to insane - instead of only
| > >>>having to test v2 vs v3 directory formats, you're suggesting we
| > >>>support v2 vs v2+dtype vs v3 vs v3+dtype. We simply do not have the
| > >>>resources to adequately test and support such an explosion of
| > >>>filesystem configurations.
| > >>>
| > >>>We've had this discussion before - new on-disk features go into the
| > >>>v5 superblock format - the v4 superblock format from this point
| > >>>onwards is essentially legacy support from an upstream development
| > >>>perspective.
| ....
| > >>>That said, there's nothing to stop anyone from backporting such a
| > >>>feature to an older kernel and maintaining it themselves - it's open
| > >>>source software. But the idea that development should be constrained
| > >>>by having to support both old and new formats is wrong - the old v4
| > >>>format should be considered stable and we need think very hard about
| > >>>changing it at all now, especially as much of the development focus
| > >>>is now shifting to taking advantage of the additions to the v5
| > >>>format....
| > >>
| > >>I guess we need more time to argue this out. It is not going into
| > >>Linux 3.12 as a crc feature only.
| > >
| > >Seriously?
| > 
| > yes seriously.
| 
| Great, another random roadblock from the XFS maintainers to deal
| with.

The addition of the file type field to directory entries is a great new feature. Your implementation of adding a "hidden" byte to the name field is especially clever. This is a feature that can benefit both dir2 and dir3 format filesystems and is completely independent of your CRC and self describing metadata feature work. I understand that you are not interested in porting the capability to dir2 format filesystems yourself and do not have the resources to provide the associated testing and support. Myself and others within SGI have discussed these issues, and we are willing to take on the work ourselves rather than have this feature go only into v5 superblock filesystems where the feature is only accessible to those who are willing to risk using experimental code. Given that SGI will be doing the work to support the file type field in dir2 format filesystems, it doesn't make sense to add the code to v5 filesystems until all of the work is complete as there could be additional considerations for the on disk changes.

We also noted that this feature should not be added to the kernel until userspace code is available to support this feature. Specifically, xfs_repair needs to validate and if needed repair the the file type field. Also, tests are needed to validate the new functionality. While I expect that you will provide this support for v5 superblock and dir3 filesystems, one of us at SGI will extend the support to v4 superblock and dir2 filesystems.

On Thu, Aug 15, 2013 at 11:50:08AM -0500, Eric Sandeen wrote:

| On 8/14/13 1:47 PM, Geoffrey Wehrman wrote:
| > On Wed, Aug 14, 2013 at 05:50:42PM +1000, Dave Chinner wrote:
| > | On Tue, Aug 13, 2013 at 10:42:32AM -0500, Mark Tinguely wrote:
| > | > On 08/12/13 19:50, Dave Chinner wrote:
| > | > >On Mon, Aug 12, 2013 at 08:25:23AM -0500, Mark Tinguely wrote:
| > | > >>On 08/11/13 19:59, Dave Chinner wrote:
| > | > >>>On Tue, Jul 30, 2013 at 02:10:32PM -0500, Mark Tinguely wrote:
| > | > >>>>On 07/19/13 01:25, Dave Chinner wrote:
| > | > >>>>>From: Dave Chinner<[email protected]>
| > | > >>>>>
| > | > >>>>>Add support for the file type field in directory entries so that
| > | > >>>>>readdir can return the type of the inode the dirent points to to
| > | > >>>>>userspace without first having to read the inode off disk.
| > | .....
| > | > >>>>>Signed-off-by: Dave Chinner<[email protected]>
| > | > >>>>>---
| > | > >>>>>
| > | > >>>>
| > | > >>>>>+static inline int xfs_sb_version_hasftype(struct xfs_sb *sbp)
| > | > >>>>>+{
| > | > >>>>>+	return XFS_SB_VERSION_NUM(sbp) == XFS_SB_VERSION_5&&
| > | > >>>>>+		xfs_sb_has_incompat_feature(sbp, XFS_SB_FEAT_INCOMPAT_FTYPE);
| > | > >>>>>  }
| > | > >>>>>
| > | > >>>>
| > | > >>>>This feature should support inode version 2 and 3.
| > | > >>>
| > | > >>>Has nothing to do with the inode version number - it has to do with
| > | > >>>the directory structure being modified.
| > | > >>>
| > | > >>>We're changing the directory structure for CRCs, and this builds on
| > | > >>>top of that. It is essentially part of the V3 directory format, and
| > | > >>>should be treated as such. Suggesting that we retrofit and support a
| > | > >>>modified v2 directory format is close to insane - instead of only
| > | > >>>having to test v2 vs v3 directory formats, you're suggesting we
| > | > >>>support v2 vs v2+dtype vs v3 vs v3+dtype. We simply do not have the
| > | > >>>resources to adequately test and support such an explosion of
| > | > >>>filesystem configurations.
| > | > >>>
| > | > >>>We've had this discussion before - new on-disk features go into the
| > | > >>>v5 superblock format - the v4 superblock format from this point
| > | > >>>onwards is essentially legacy support from an upstream development
| > | > >>>perspective.
| > | ....
| > | > >>>That said, there's nothing to stop anyone from backporting such a
| > | > >>>feature to an older kernel and maintaining it themselves - it's open
| > | > >>>source software. But the idea that development should be constrained
| > | > >>>by having to support both old and new formats is wrong - the old v4
| > | > >>>format should be considered stable and we need think very hard about
| > | > >>>changing it at all now, especially as much of the development focus
| > | > >>>is now shifting to taking advantage of the additions to the v5
| > | > >>>format....
| > | > >>
| > | > >>I guess we need more time to argue this out. It is not going into
| > | > >>Linux 3.12 as a crc feature only.
| > | > >
| > | > >Seriously?
| > | > 
| > | > yes seriously.
| > | 
| > | Great, another random roadblock from the XFS maintainers to deal
| > | with.
| > 
| > The addition of the file type field to directory entries is a great
| > new feature.  Your implementation of adding a "hidden" byte to the name
| > field is especially clever.  This is a feature that can benefit both
| > dir2 and dir3 format filesystems and is completely independent of your
| > CRC and self describing metadata feature work.  I understand that you
| > are not interested in porting the capability to dir2 format filesystems
| > yourself and do not have the resources to provide the associated testing
| > and support.  Myself and others within SGI have discussed these issues,
| > and we are willing to take on the work ourselves rather than have this
| > feature go only into v5 superblock filesystems where the feature is only
| > accessible to those who are willing to risk using experimental code.
| > Given that SGI will be doing the work to support the file type field
| > in dir2 format filesystems, it doesn't make sense to add the code to
| > v5 filesystems until all of the work is complete as there could be
| > additional considerations for the on disk changes.
| > 
| > We also noted that this feature should not be added to the kernel until
| > userspace code is available to support this feature.  Specifically,
| > xfs_repair needs to validate and if needed repair the the file type field.
| > Also, tests are needed to validate the new functionality.  While I
| > expect that you will provide this support for v5 superblock and dir3
| > filesystems, one of us at SGI will extend the support to v4 superblock
| > and dir2 filesystems.
| 
| These requirements are very, very late in the process.  Dave's work has
| been discussed for a long time in public, with plenty of time for input
| and cooperation and coordination.
| 
| If the type field is critical to SGI on V4 superblocks, I'd have expected
| to see patches from SGI by now, either prior to, or in coordination with,
| Dave's work.  So it's hard for me to tell if this is a strategic requirement
| for you, or an effort to delay the CRC work which you seem to be uneasy with.
| Indeed, you floated this as a requirement many weeks (months?) ago, but as far
| as I know, no actual work or patches or proposals to implement it have been
| forthcoming from SGI.
| 
| I apologize for not being up to speed on the technical details of what it
| might take, but I figure surely SGI is on top of it, if you're signing up
| to do the work.
| 
| So I'd propose (and the guys in the trenches can bang this around) that if
| you are serious about this, you commit to producing patches which address your
| stated requirements without negatively affecting Dave's V5 design, with all
| necessary retesting, any of Dave's outstanding patches rebased as necessary,
| and everything ready for upstream integration on the original schedule.
| 
| You've had plenty of time to do this, but it's not been done AFAICT.
| I think you need to get busy and back up your words with patches pronto, or drop
| the above stated requirement; perhaps there is a way to add the support to V4
| superblocks retroactively if you miss this window.
| 
| There may well be technical hurdles I'm not aware of, but I think we need to see
| SGI's proposed implementation to be able to discuss that efficiently.

I'll admit that I'm not a close follower of the oss-xfs mail list, but the first mention I can find of the dirent filetype field feature on the list is in Dave's patch posted July 19 along with 48 other patches.
http://oss.sgi.com/archives/xfs/2013-07/msg00422.html

If there was discussion on the list about this feature prior to the above posting, I apologize. Had I known the dirent filetype feature was coming and would not be supported in v4 filesystems, I would have been able to start this conversation earlier.

Quoted 78 lines · Geoffrey Wehrman

On Wed, Aug 14, 2013 at 05:50:42PM +1000, Dave Chinner wrote:

| On Tue, Aug 13, 2013 at 10:42:32AM -0500, Mark Tinguely wrote:
| > On 08/12/13 19:50, Dave Chinner wrote:
| > >On Mon, Aug 12, 2013 at 08:25:23AM -0500, Mark Tinguely wrote:
| > >>On 08/11/13 19:59, Dave Chinner wrote:
| > >>>On Tue, Jul 30, 2013 at 02:10:32PM -0500, Mark Tinguely wrote:
| > >>>>On 07/19/13 01:25, Dave Chinner wrote:
| > >>>>>From: Dave Chinner<[email protected]>
| > >>>>>
| > >>>>>Add support for the file type field in directory entries so that
| > >>>>>readdir can return the type of the inode the dirent points to to
| > >>>>>userspace without first having to read the inode off disk.
| .....
| > >>>>>Signed-off-by: Dave Chinner<[email protected]>
| > >>>>>---
| > >>>>>
| > >>>>
| > >>>>>+static inline int xfs_sb_version_hasftype(struct xfs_sb *sbp)
| > >>>>>+{
| > >>>>>+	return XFS_SB_VERSION_NUM(sbp) == XFS_SB_VERSION_5&&
| > >>>>>+		xfs_sb_has_incompat_feature(sbp, XFS_SB_FEAT_INCOMPAT_FTYPE);
| > >>>>>  }
| > >>>>>
| > >>>>
| > >>>>This feature should support inode version 2 and 3.
| > >>>
| > >>>Has nothing to do with the inode version number - it has to do with
| > >>>the directory structure being modified.
| > >>>
| > >>>We're changing the directory structure for CRCs, and this builds on
| > >>>top of that. It is essentially part of the V3 directory format, and
| > >>>should be treated as such. Suggesting that we retrofit and support a
| > >>>modified v2 directory format is close to insane - instead of only
| > >>>having to test v2 vs v3 directory formats, you're suggesting we
| > >>>support v2 vs v2+dtype vs v3 vs v3+dtype. We simply do not have the
| > >>>resources to adequately test and support such an explosion of
| > >>>filesystem configurations.
| > >>>
| > >>>We've had this discussion before - new on-disk features go into the
| > >>>v5 superblock format - the v4 superblock format from this point
| > >>>onwards is essentially legacy support from an upstream development
| > >>>perspective.
| ....
| > >>>That said, there's nothing to stop anyone from backporting such a
| > >>>feature to an older kernel and maintaining it themselves - it's open
| > >>>source software. But the idea that development should be constrained
| > >>>by having to support both old and new formats is wrong - the old v4
| > >>>format should be considered stable and we need think very hard about
| > >>>changing it at all now, especially as much of the development focus
| > >>>is now shifting to taking advantage of the additions to the v5
| > >>>format....
| > >>
| > >>I guess we need more time to argue this out. It is not going into
| > >>Linux 3.12 as a crc feature only.
| > >
| > >Seriously?
| > 
| > yes seriously.
| 
| Great, another random roadblock from the XFS maintainers to deal
| with.

The addition of the file type field to directory entries is a great new feature. Your implementation of adding a "hidden" byte to the name field is especially clever. This is a feature that can benefit both dir2 and dir3 format filesystems and is completely independent of your CRC and self describing metadata feature work. I understand that you are not interested in porting the capability to dir2 format filesystems yourself and do not have the resources to provide the associated testing and support. Myself and others within SGI have discussed these issues, and we are willing to take on the work ourselves rather than have this feature go only into v5 superblock filesystems where the feature is only accessible to those who are willing to risk using experimental code. Given that SGI will be doing the work to support the file type field in dir2 format filesystems, it doesn't make sense to add the code to v5 filesystems until all of the work is complete as there could be additional considerations for the on disk changes.

Quoted 7 lines

We also noted that this feature should not be added to the kernel until userspace code is available to support this feature. Specifically, xfs_repair needs to validate and if needed repair the the file type field. Also, tests are needed to validate the new functionality. While I expect that you will provide this support for v5 superblock and dir3 filesystems, one of us at SGI will extend the support to v4 superblock and dir2 filesystems.

These requirements are very, very late in the process. Dave's work has been discussed for a long time in public, with plenty of time for input and cooperation and coordination.

If the type field is critical to SGI on V4 superblocks, I'd have expected to see patches from SGI by now, either prior to, or in coordination with, Dave's work. So it's hard for me to tell if this is a strategic requirement for you, or an effort to delay the CRC work which you seem to be uneasy with.

Indeed, you floated this as a requirement many weeks (months?) ago, but as far as I know, no actual work or patches or proposals to implement it have been forthcoming from SGI.

I apologize for not being up to speed on the technical details of what it might take, but I figure surely SGI is on top of it, if you're signing up to do the work.

So I'd propose (and the guys in the trenches can bang this around) that if you are serious about this, you commit to producing patches which address your stated requirements without negatively affecting Dave's V5 design, with all necessary retesting, any of Dave's outstanding patches rebased as necessary, and everything ready for upstream integration on the original schedule.

From: Dave Chinner <[email protected]>

Add support for the file type field in directory entries so that readdir can return the type of the inode the dirent points to to userspace without first having to read the inode off disk.

The encoding of the type field is a single byte that is added to the end of the directory entry name length. For all intents and purposes, it appends a "hidden" byte to the name field which contains the type information. As the directory entry is already of dynamic size, helpers are already required to access and decode the direct entry structures.

Hence the relevent extraction and iteration helpers are updated to understand the hidden byte. Helpers for reading and writing the filetype field from the directory entries are also added. Only the read helpers are used by this patch. It also adds all the code necessary to read the type information out of the dirents on disk.

Further we add the superblock feature bit and helpers to indicate that we understand the on-disk format change. This is not a compatible change - existing kernels cannot read the new format successfully - so an incompatible feature flag is added. We don't yet allow filesystems to mount with this flag yet - that will be added once write support is added.

Finally, the code to take the type from the VFS, convert it to an XFS on-disk type and put it into the xfs_name structures passed around is added, but the directory code does not use this field yet. That will be in the next patch.

Signed-off-by Dave Chinner <[email protected]>

Hi folks,

The following mass of patches is my current patch queue I have pending for the 3.12 cycle.

Quoted 4 lines · Dave Chinner

Hi folks,

The following mass of patches is my current patch queue I have pending for the 3.12 cycle.

OK, it seems to be going okay: The PC is still functioning, even on a CRC-enabled / partition. The git am session had these anomalies:

# Patch #5
Applying: xfs: separate dquot on disk format definitions out of xfs_quota.h /usr/src/kernel-git/linux/.git/rebase-apply/patch:138: trailing whitespace.
* This header file defines all the on-disk format definitions for
warning: 1 line adds whitespace errors.

# Patch #15
Applying: xfs: move getdents code into it's own file /usr/src/kernel-git/linux/.git/rebase-apply/patch:1266: new blank line at EOF. +
warning: 1 line adds whitespace errors.

########
There were build errors as well with gcc-4.8.1:

CC fs/xfs/xfs_inode_fork.o
fs/xfs/xfs_inode_fork.c:43:23: fatal error: xfs_utils.h: No such file or directory

 #include "xfs_utils.h"
                       ^

compilation terminated.
make[2]: *** [fs/xfs/xfs_inode_fork.o] Error 1
make[1]: *** [fs/xfs] Error 2
make: *** [fs] Error 2

# next try...
  CC      fs/xfs/xfs_inode_fork.o
fs/xfs/xfs_inode_fork.c:49:26: fatal error: xfs_vnodeops.h: No such file or directory
 #include "xfs_vnodeops.h"

Quoted 28 lines · Michael L. Semon

Quoted 4 lines · Dave Chinner

Hi folks,

The following mass of patches is my current patch queue I have pending for the 3.12 cycle.

OK, it seems to be going okay: The PC is still functioning, even on a CRC-enabled / partition. The git am session had these anomalies:

# Patch #5
Applying: xfs: separate dquot on disk format definitions out of xfs_quota.h /usr/src/kernel-git/linux/.git/rebase-apply/patch:138: trailing whitespace.
* This header file defines all the on-disk format definitions for
warning: 1 line adds whitespace errors.

# Patch #15
Applying: xfs: move getdents code into it's own file /usr/src/kernel-git/linux/.git/rebase-apply/patch:1266: new blank line at EOF. +
warning: 1 line adds whitespace errors.

########
There were build errors as well with gcc-4.8.1:

CC fs/xfs/xfs_inode_fork.o
fs/xfs/xfs_inode_fork.c:43:23: fatal error: xfs_utils.h: No such file or directory

 #include "xfs_utils.h"
                       ^

Quoted 8 lines

compilation terminated.
make[2]: *** [fs/xfs/xfs_inode_fork.o] Error 1
make[1]: *** [fs/xfs] Error 2
make: *** [fs] Error 2

# next try...
  CC      fs/xfs/xfs_inode_fork.o
fs/xfs/xfs_inode_fork.c:49:26: fatal error: xfs_vnodeops.h: No such file or directory
 #include "xfs_vnodeops.h"

Ah, I thought I caught all of them. I use a rsync'd build tree, and I don't use --delete because that removes all the object files. Hence sometimes I end up with "stale files" that have been removed from the source tree but don't get removed from the build tree and so the build doesn't fail.

(I thought I added a "git clean -f -d" to my build script, but on review, that only went into the userspace builds....)

fs/xfs/xfs_log_rlimit.c xfs_log_calc_minimum_size( · line 141

Quoted 9 lines

fs/xfs/xfs_log_rlimit.c +2 -1

@@ -137,7 +137,8 @@ xfs_log_calc_minimum_size(
       * it up to lsunit boundary if lsunit is specified.
       */
      if (lsunit)
-             min_logblks = roundup(BTOBB(max_logres), lsunit) + 2 * lsunit;
+             min_logblks = roundup((int)BTOBB(max_logres), lsunit)
+                     + 2 * lsunit;

Why did you need that one change?