[DISCUSS] Planning for new dev cycle (3.17)

[DISCUSS] Planning for new dev cycle (3.17)

[Dave Chinner]

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

Hi everyone,

Now that the 3.16 dev cycle has drawn to a close (one more
linux-next build and I'll tag for-next and send a pull request),
it's time to look ahead for the next couple of months. I think the
current major pieces of work that are currently outstanding are
these:

	- Jeff's bulkstat rework
	- Brian's EOF prealloc scanning
	- Namjae's FALLOC_FL_INSERT_RANGE work
	- Eric's XFS_ERROR() macro removal and return () cleanup.

There's also two major pieces of infrastructure work I'd like to get
done:

	- convert XFS to negative error returns
	- restructure code to have a fs/xfs/libxfs structure similar
	  to userspace

Because Eric's XFS_ERROR removal touches the entire codebase, as
does the negative error return and the libxfs restructuring, I'd
like to get these done first and base the rest of the dev cycle work
on top of that. Eric's patches just need a minor rebase and the
libxfs restructure needs some makefile rework and review and they
should be good to go.

The issue is the negative error number patchset, and how to handle
review and testing. The patchset is already 62 patches long and it
converts roughly half the code base. It'll take me another couple of
days to convert the rest of the code, and that will probably take
another 60 patches.

I understand that reviewing 100+ patches is going to be a pain, but
each patch currently averages about +/- 10 lines. The current
diffstat is:

	37 files changed, 723 insertions(+), 722 deletions(-)

And that will probably double, so it's still going to be a fair
amount of change.

So the big question is how do we handle the review side of things.
I think testing won't be a huge issue because of the time we have in
the cycle (a couple of months to the 3.17 merge, and then a couple
more months in the 3.17-rcX cycle) to find and catch regressions,
but I'd like to know what people think about the best way to review
this change will be.

I'm happy for people to say "no, we need to review it patch by
patch, so delay it for a cycle while we work through it", but I'm
also happy for a "apply it all and look at error sources and
inversion points for problems". The second is probably easier, as
there will be very few remaining inversion points (only embedded
errors in ioctl structures, I think) and all error sources should be
negated at their definition and hence any error value (E* values)
that are not defined as "-E*" is likely to be an mistake....

I'll be spending the next couple of days finishing this all off, so
once it is done I can focus on review and bug fixes for the rest of
the 3.17 dev cycle. That allows me to  concentrate on a xfsprogs
3.2.1 release, subsequent userspace libxfs resync with the new
kernel structure, and starting to work on some of the smart block
device concepts I've been talking about recently....

These are not concrete plans - just what I'd *like* to do in the
next couple of months. Reality is bound to mess up any plans I have,
so I figure I mays as well mess them up straight away....

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

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

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

Re: [DISCUSS] Planning for new dev cycle (3.17)

[Dave Chinner]

On Tue, Jun 10, 2014 at 08:33:20AM +1000, Dave Chinner wrote:
> Hi everyone,
> 
> Now that the 3.16 dev cycle has drawn to a close (one more
> linux-next build and I'll tag for-next and send a pull request),
> it's time to look ahead for the next couple of months. I think the
> current major pieces of work that are currently outstanding are
> these:
> 
> 	- Jeff's bulkstat rework
> 	- Brian's EOF prealloc scanning
> 	- Namjae's FALLOC_FL_INSERT_RANGE work
> 	- Eric's XFS_ERROR() macro removal and return () cleanup.
> 
> There's also two major pieces of infrastructure work I'd like to get
> done:
> 
> 	- convert XFS to negative error returns
> 	- restructure code to have a fs/xfs/libxfs structure similar
> 	  to userspace
> 
> Because Eric's XFS_ERROR removal touches the entire codebase, as
> does the negative error return and the libxfs restructuring, I'd
> like to get these done first and base the rest of the dev cycle work
> on top of that. Eric's patches just need a minor rebase and the
> libxfs restructure needs some makefile rework and review and they
> should be good to go.

There is a new version of this work (Eric's patches and the base
libxfs restructure) here:

	git://oss.sgi.com/xfs/xfs.git xfs-libxfs-restructure

It's based on 3.15 with the current for-next branch merged into it
and then the changes done over the top, so it's about as up-to-date
as it can be. Comments welcome...

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

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

Re: [DISCUSS] Planning for new dev cycle (3.17)

[Brian Foster]

On Tue, Jun 10, 2014 at 08:33:20AM +1000, Dave Chinner wrote:
> Hi everyone,
> 
> Now that the 3.16 dev cycle has drawn to a close (one more
> linux-next build and I'll tag for-next and send a pull request),
> it's time to look ahead for the next couple of months. I think the
> current major pieces of work that are currently outstanding are
> these:
> 
> 	- Jeff's bulkstat rework
> 	- Brian's EOF prealloc scanning
> 	- Namjae's FALLOC_FL_INSERT_RANGE work
> 	- Eric's XFS_ERROR() macro removal and return () cleanup.
> 

I'm not tied to a particular kernel release by any means if there's
already a lot in the pipeline, but I'd like to include the basic sysfs
bits somewhere in the tail end of this.

> There's also two major pieces of infrastructure work I'd like to get
> done:
> 
> 	- convert XFS to negative error returns
> 	- restructure code to have a fs/xfs/libxfs structure similar
> 	  to userspace
> 
> Because Eric's XFS_ERROR removal touches the entire codebase, as
> does the negative error return and the libxfs restructuring, I'd
> like to get these done first and base the rest of the dev cycle work
> on top of that. Eric's patches just need a minor rebase and the
> libxfs restructure needs some makefile rework and review and they
> should be good to go.
> 

Sounds reasonable to me.

> The issue is the negative error number patchset, and how to handle
> review and testing. The patchset is already 62 patches long and it
> converts roughly half the code base. It'll take me another couple of
> days to convert the rest of the code, and that will probably take
> another 60 patches.
> 
> I understand that reviewing 100+ patches is going to be a pain, but
> each patch currently averages about +/- 10 lines. The current
> diffstat is:
> 
> 	37 files changed, 723 insertions(+), 722 deletions(-)
> 
> And that will probably double, so it's still going to be a fair
> amount of change.
> 

Is there any sort of more coarse logical breakdown of this series, or do
we want/need to convert the entire codebase all at once? The individual
patches sound relatively small... is there a particular method at play
there? E.g., a patch per function? file? call chain?

> So the big question is how do we handle the review side of things.
> I think testing won't be a huge issue because of the time we have in
> the cycle (a couple of months to the 3.17 merge, and then a couple
> more months in the 3.17-rcX cycle) to find and catch regressions,
> but I'd like to know what people think about the best way to review
> this change will be.
> 
> I'm happy for people to say "no, we need to review it patch by
> patch, so delay it for a cycle while we work through it", but I'm
> also happy for a "apply it all and look at error sources and
> inversion points for problems". The second is probably easier, as
> there will be very few remaining inversion points (only embedded
> errors in ioctl structures, I think) and all error sources should be
> negated at their definition and hence any error value (E* values)
> that are not defined as "-E*" is likely to be an mistake....
> 

Personally, I generally prefer going through individual patches. Even if
we don't post the entire series and do a patch-by-patch reviewed-by
(which sounds like overkill in this case), that helps me do bits a time,
keep track of where I am, etc. I say that before I've seen any of these
patches of course ;), so I could certainly see running through some kind
of approach of doing it batches (i.e., "look at this sequence of
patches, identify the affected segments of code, make a direct pass
through that code, repeat"). I guess it's hard to say without just
digging in and finding the most effective approach to get through it.

Perhaps if we just make a branch available with the patches, put a
notification on the list, and we can use that as a review thread..?

Brian

> I'll be spending the next couple of days finishing this all off, so
> once it is done I can focus on review and bug fixes for the rest of
> the 3.17 dev cycle. That allows me to  concentrate on a xfsprogs
> 3.2.1 release, subsequent userspace libxfs resync with the new
> kernel structure, and starting to work on some of the smart block
> device concepts I've been talking about recently....
> 
> These are not concrete plans - just what I'd *like* to do in the
> next couple of months. Reality is bound to mess up any plans I have,
> so I figure I mays as well mess them up straight away....
> 
> Cheers,
> 
> Dave.
> -- 
> Dave Chinner
> david@fromorbit.com



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

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

Re: [DISCUSS] Planning for new dev cycle (3.17)

[Eric Sandeen]

On 6/10/14, 1:09 AM, Dave Chinner wrote:

> There is a new version of this work (Eric's patches and the base
> libxfs restructure) here:
> 
> 	git://oss.sgi.com/xfs/xfs.git xfs-libxfs-restructure
> 
> It's based on 3.15 with the current for-next branch merged into it
> and then the changes done over the top, so it's about as up-to-date
> as it can be. Comments welcome...

Comment 1: doesn't build ;)


make[1]: *** No rule to make target `fs/xfs/xfs_dir2_readdir.o', needed by `fs/xfs/xfs.o'.  Stop.
make[1]: *** Waiting for unfinished jobs....
  CC [M]  fs/xfs/xfs_buf.o
make: *** [_module_fs/xfs] Error 2

problems w/ xfs_rtbitmap.o as well, patch follows.

Comment 2: Coverity thinks this adds about 25 defects, I'll have to go
look at why...

-Eric

diff --git a/fs/xfs/Makefile b/fs/xfs/Makefile
index a22a6b8..0b7b3b3 100644
--- a/fs/xfs/Makefile
+++ b/fs/xfs/Makefile
@@ -43,6 +43,7 @@ xfs-y				+= $(addprefix libxfs/, \
 				   xfs_dir2_data.o \
 				   xfs_dir2_leaf.o \
 				   xfs_dir2_node.o \
+				   xfs_dir2_readdir.o \
 				   xfs_dir2_sf.o \
 				   xfs_dquot_buf.o \
 				   xfs_ialloc.o \
@@ -55,6 +56,9 @@ xfs-y				+= $(addprefix libxfs/, \
 				   xfs_trans_resv.o \
 				   )
 
+xfs-$(CONFIG_XFS_RT)		+= $(addprefix libxfs/, \
+				   xfs_rtbitmap.o \
+				   )
 # highlevel code
 xfs-y				+= xfs_aops.o \
 				   xfs_attr_inactive.o \
@@ -62,7 +66,6 @@ xfs-y				+= xfs_aops.o \
 				   xfs_bit.o \
 				   xfs_bmap_util.o \
 				   xfs_buf.o \
-				   xfs_dir2_readdir.o \
 				   xfs_discard.o \
 				   xfs_error.o \
 				   xfs_export.o \
@@ -110,8 +113,7 @@ xfs-$(CONFIG_XFS_QUOTA)		+= xfs_dquot.o \
 				   xfs_quotaops.o
 
 # xfs_rtbitmap is shared with libxfs
-xfs-$(CONFIG_XFS_RT)		+= xfs_rtalloc.o \
-				   xfs_rtbitmap.o
+xfs-$(CONFIG_XFS_RT)		+= xfs_rtalloc.o
 
 xfs-$(CONFIG_XFS_POSIX_ACL)	+= xfs_acl.o
 xfs-$(CONFIG_PROC_FS)		+= xfs_stats.o




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

Re: [DISCUSS] Planning for new dev cycle (3.17)

[Dave Chinner]

On Tue, Jun 10, 2014 at 07:58:57AM -0400, Brian Foster wrote:
> On Tue, Jun 10, 2014 at 08:33:20AM +1000, Dave Chinner wrote:
> > Hi everyone,
> > 
> > Now that the 3.16 dev cycle has drawn to a close (one more
> > linux-next build and I'll tag for-next and send a pull request),
> > it's time to look ahead for the next couple of months. I think the
> > current major pieces of work that are currently outstanding are
> > these:
> > 
> > 	- Jeff's bulkstat rework
> > 	- Brian's EOF prealloc scanning
> > 	- Namjae's FALLOC_FL_INSERT_RANGE work
> > 	- Eric's XFS_ERROR() macro removal and return () cleanup.
> > 
> 
> I'm not tied to a particular kernel release by any means if there's
> already a lot in the pipeline, but I'd like to include the basic sysfs
> bits somewhere in the tail end of this.

Yes, I don't see a problem there - I simply forgot about that ;)

> > There's also two major pieces of infrastructure work I'd like to get
> > done:
> > 
> > 	- convert XFS to negative error returns
> > 	- restructure code to have a fs/xfs/libxfs structure similar
> > 	  to userspace
> > 
> > Because Eric's XFS_ERROR removal touches the entire codebase, as
> > does the negative error return and the libxfs restructuring, I'd
> > like to get these done first and base the rest of the dev cycle work
> > on top of that. Eric's patches just need a minor rebase and the
> > libxfs restructure needs some makefile rework and review and they
> > should be good to go.
> > 
> 
> Sounds reasonable to me.
> 
> > The issue is the negative error number patchset, and how to handle
> > review and testing. The patchset is already 62 patches long and it
> > converts roughly half the code base. It'll take me another couple of
> > days to convert the rest of the code, and that will probably take
> > another 60 patches.
> > 
> > I understand that reviewing 100+ patches is going to be a pain, but
> > each patch currently averages about +/- 10 lines. The current
> > diffstat is:
> > 
> > 	37 files changed, 723 insertions(+), 722 deletions(-)
> > 
> > And that will probably double, so it's still going to be a fair
> > amount of change.
> 
> Is there any sort of more coarse logical breakdown of this series, or do
> we want/need to convert the entire codebase all at once? The individual
> patches sound relatively small... is there a particular method at play
> there? E.g., a patch per function? file? call chain?

I'm doing it layer by layer, starting from the linux interface
layers and working my way down. e.g. fs/xfs/xfs_file.c first,
the fs/xfs/xfs_iops.c, and so on, and there are multiple patches per
file for each (roughly) logical change. e.g. converting xfs_iops.c:

[these are in reverse order because of git log listing]

1f80bd7 xfs: convert error sign for xfs_setattr
794c21d xfs: negate error from xfs_rename
032669b xfs: negate error for link/unlink/symlink
a12b6b1 xfs: negate error from xfs_lookup
a0f5650 xfs: push negative errno down through xfs_generic_create

pushes the conversion layer to fs/xfs/xfs_inode.c.  Then later the
quota bits stuff like xfs_create() call are converted:

350747a xfs: negate error retursn from dquot attach/detach functions

as the negative errors are driven inward in the dquot
infrastructure, slowly removing all the conversions from the
fs/xfs/xfs_inode.c layer, Similarly, the directory code is converted
in a similarly layered approach

....
f3086a1 xfs: convert shortform directories to negative errors
bbda37c xfs: negate dir inode grow/shrink return values
7f73c2b xfs: convert remaining directory name operations to negative error
bca2fa3 xfs: negate error values from directory create functions

And so on. Basically once all the high layer functions are
converted, I'll move onto the inner infrastructure such as the
xfs_bmap code, then the btree code, then the xfs_trans code, then
the xfs_buf, and so on until all the code is using negative errors
and the only conversions are in the ioctl code for userspace
presentation.

IMO, the only way to guarantee that we've got it right is to
convert everything - if we stop half way, then all we've done is
move the interface layer and made it extremely hard to validate that
the interface layer is correct. Hence I don't want to do that - I
just want to change it all in one go to make it easier to validate
the end result...

> > So the big question is how do we handle the review side of
> > things.  I think testing won't be a huge issue because of the
> > time we have in the cycle (a couple of months to the 3.17 merge,
> > and then a couple more months in the 3.17-rcX cycle) to find and
> > catch regressions, but I'd like to know what people think about
> > the best way to review this change will be.
> > 
> > I'm happy for people to say "no, we need to review it patch by
> > patch, so delay it for a cycle while we work through it", but
> > I'm also happy for a "apply it all and look at error sources and
> > inversion points for problems". The second is probably easier,
> > as there will be very few remaining inversion points (only
> > embedded errors in ioctl structures, I think) and all error
> > sources should be negated at their definition and hence any
> > error value (E* values) that are not defined as "-E*" is likely
> > to be an mistake....
> > 
> 
> Personally, I generally prefer going through individual patches.
> Even if we don't post the entire series and do a patch-by-patch
> reviewed-by (which sounds like overkill in this case), that helps
> me do bits a time, keep track of where I am, etc. I say that
> before I've seen any of these patches of course ;), so I could
> certainly see running through some kind of approach of doing it
> batches (i.e., "look at this sequence of patches, identify the
> affected segments of code, make a direct pass through that code,
> repeat"). I guess it's hard to say without just digging in and
> finding the most effective approach to get through it.
> 
> Perhaps if we just make a branch available with the patches, put a
> notification on the list, and we can use that as a review
> thread..?

I'll push the series to the git tree at the end of the day - I'm
hoping to have the conversion mostly done by then. I did most of the
rebase of the existing patchset on top of the libxfs addition last
night, so I should e able to knock off most of the rest of the
changes today. I wanted to see what people thought about the concept
without cluttering the issue with a huge code dump...

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

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

Re: [DISCUSS] Planning for new dev cycle (3.17)

[Dave Chinner]

On Tue, Jun 10, 2014 at 09:09:08AM -0500, Eric Sandeen wrote:
> On 6/10/14, 1:09 AM, Dave Chinner wrote:
> 
> > There is a new version of this work (Eric's patches and the base
> > libxfs restructure) here:
> > 
> > 	git://oss.sgi.com/xfs/xfs.git xfs-libxfs-restructure
> > 
> > It's based on 3.15 with the current for-next branch merged into it
> > and then the changes done over the top, so it's about as up-to-date
> > as it can be. Comments welcome...
> 
> Comment 1: doesn't build ;)
> 
> 
> make[1]: *** No rule to make target `fs/xfs/xfs_dir2_readdir.o', needed by `fs/xfs/xfs.o'.  Stop.
> make[1]: *** Waiting for unfinished jobs....
>   CC [M]  fs/xfs/xfs_buf.o
> make: *** [_module_fs/xfs] Error 2

Ah, that's an issue....

> problems w/ xfs_rtbitmap.o as well, patch follows.
> 
> Comment 2: Coverity thinks this adds about 25 defects, I'll have to go
> look at why...

Probably because it ignores issues based on path/filename, and the
path changed...

> 
> -Eric
> 
> diff --git a/fs/xfs/Makefile b/fs/xfs/Makefile
> index a22a6b8..0b7b3b3 100644
> --- a/fs/xfs/Makefile
> +++ b/fs/xfs/Makefile
> @@ -43,6 +43,7 @@ xfs-y				+= $(addprefix libxfs/, \
>  				   xfs_dir2_data.o \
>  				   xfs_dir2_leaf.o \
>  				   xfs_dir2_node.o \
> +				   xfs_dir2_readdir.o \

No, the xfs_dir2_readdir.c file should not have been moved - it's
kernel only functionality.

>  				   xfs_dir2_sf.o \
>  				   xfs_dquot_buf.o \
>  				   xfs_ialloc.o \
> @@ -55,6 +56,9 @@ xfs-y				+= $(addprefix libxfs/, \
>  				   xfs_trans_resv.o \
>  				   )
>  
> +xfs-$(CONFIG_XFS_RT)		+= $(addprefix libxfs/, \
> +				   xfs_rtbitmap.o \
> +				   )

But that needs fixing, yes.

Makes me wonder why it linked here - must have found stale .o files
in the build area and linked them even though the files didn't get
built.

Anyway, I'll fix it, and restart the errno negation rebase....

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

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

Re: [DISCUSS] Planning for new dev cycle (3.17)

[Eric Sandeen]

On 6/10/14, 9:09 AM, Eric Sandeen wrote:
> On 6/10/14, 1:09 AM, Dave Chinner wrote:
> 
>> There is a new version of this work (Eric's patches and the base
>> libxfs restructure) here:
>>
>> 	git://oss.sgi.com/xfs/xfs.git xfs-libxfs-restructure
>>
>> It's based on 3.15 with the current for-next branch merged into it
>> and then the changes done over the top, so it's about as up-to-date
>> as it can be. Comments welcome...
> 
> Comment 1: doesn't build ;)
> 
> 
> make[1]: *** No rule to make target `fs/xfs/xfs_dir2_readdir.o', needed by `fs/xfs/xfs.o'.  Stop.
> make[1]: *** Waiting for unfinished jobs....
>   CC [M]  fs/xfs/xfs_buf.o
> make: *** [_module_fs/xfs] Error 2
> 
> problems w/ xfs_rtbitmap.o as well, patch follows.
> 
> Comment 2: Coverity thinks this adds about 25 defects, I'll have to go
> look at why...

(whoops, I forgot to send this earlier)

I take that back, somehow it was still counting defects in the files that got moved.

At most it added one, but really this code is the same as upstream, so
it's probably just a coverity foible; the issue it finds seems real though,
I'll send a patch:

Error: FORWARD_NULL:
path:/mnt/test2/git/xfs/fs/xfs/libxfs/xfs_da_btree.c:2555:
cond_false: Condition "error", taking false branch
path:/mnt/test2/git/xfs/fs/xfs/libxfs/xfs_da_btree.c:2560:
if_end: End of if statement
path:/mnt/test2/git/xfs/fs/xfs/libxfs/xfs_da_btree.c:2564:
cond_false: Condition "bp", taking false branch
/mnt/test2/git/xfs/fs/xfs/libxfs/xfs_da_btree.c:2564:
var_compare_op: Comparing "bp" to null implies that "bp" might be null.
path:/mnt/test2/git/xfs/fs/xfs/libxfs/xfs_da_btree.c:2565:
cond_true: Condition "error", taking true branch
/mnt/test2/git/xfs/fs/xfs/libxfs/xfs_da_btree.c:2566:
var_deref_model: Passing null pointer "bp" to function "xfs_trans_brelse(xfs_trans_t *, xfs_buf_t *)", which dereferences it.
path:/mnt/test2/git/xfs/fs/xfs/xfs_trans_buf.c:443:2:
cond_true: Condition "tp == NULL", taking true branch
/mnt/test2/git/xfs/fs/xfs/xfs_trans_buf.c:444:3:
deref_parm: Directly dereferencing parameter "bp".

-Eric

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

Re: [DISCUSS] Planning for new dev cycle (3.17)

[Dave Chinner]

On Wed, Jun 11, 2014 at 07:48:50AM +1000, Dave Chinner wrote:
> On Tue, Jun 10, 2014 at 07:58:57AM -0400, Brian Foster wrote:
> > On Tue, Jun 10, 2014 at 08:33:20AM +1000, Dave Chinner wrote:
> > > The issue is the negative error number patchset, and how to handle
> > > review and testing. The patchset is already 62 patches long and it
> > > converts roughly half the code base. It'll take me another couple of
> > > days to convert the rest of the code, and that will probably take
> > > another 60 patches.
> > > 
> > > I understand that reviewing 100+ patches is going to be a pain, but
> > > each patch currently averages about +/- 10 lines. The current
> > > diffstat is:
> > > 
> > > 	37 files changed, 723 insertions(+), 722 deletions(-)
> > > 
> > > And that will probably double, so it's still going to be a fair
> > > amount of change.
> > 
> > Is there any sort of more coarse logical breakdown of this series, or do
> > we want/need to convert the entire codebase all at once? The individual
> > patches sound relatively small... is there a particular method at play
> > there? E.g., a patch per function? file? call chain?
> 
> I'm doing it layer by layer, starting from the linux interface
> layers and working my way down. e.g. fs/xfs/xfs_file.c first,
> the fs/xfs/xfs_iops.c, and so on, and there are multiple patches per
> file for each (roughly) logical change. e.g. converting xfs_iops.c:
[...]

I've decided that there really is too much unnecessary code churn
from this approach. I end up converting all the call sites to negate
the error sign, and then end up converting them back to the original
code some time later, leaving only the source of the errors with a
changed sign.

So, I stopped doing that to see just what the brute force, change
source and conversions only, and I found with a few simple searches
I could identify all the locations that need changing. So, in a
couple of hours I churned out the patch that converted everything.
Still pretty large, even though it only changes error values and
conversion points.

67 files changed, 879 insertions(+), 884 deletions(-)

Not sure how I could break this up - it really is an all-or-nothing
patch this Big Hammer approach....

> > Perhaps if we just make a branch available with the patches, put a
> > notification on the list, and we can use that as a review
> > thread..?
> 
> I'll push the series to the git tree at the end of the day - I'm
> hoping to have the conversion mostly done by then. I did most of the
> rebase of the existing patchset on top of the libxfs addition last
> night, so I should e able to knock off most of the rest of the
> changes today. I wanted to see what people thought about the concept
> without cluttering the issue with a huge code dump...

Ok, so the version I pushed to the rebased xfs-libxfs-restructure
branch is the big hammer patch from above (commit fcec2eb "xfs:
global error sign conversion"). I also folded in the fix for the
problem Eric pointed out (which is why it's a rebase).  That branch
is running xfstests right now on several machines and hasn't gone
boom, so i can't have screwed it up too badly...

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

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

Re: [DISCUSS] Planning for new dev cycle (3.17)

[Brian Foster]

On Wed, Jun 11, 2014 at 07:10:20PM +1000, Dave Chinner wrote:
> On Wed, Jun 11, 2014 at 07:48:50AM +1000, Dave Chinner wrote:
> > On Tue, Jun 10, 2014 at 07:58:57AM -0400, Brian Foster wrote:
> > > On Tue, Jun 10, 2014 at 08:33:20AM +1000, Dave Chinner wrote:
> > > > The issue is the negative error number patchset, and how to handle
> > > > review and testing. The patchset is already 62 patches long and it
> > > > converts roughly half the code base. It'll take me another couple of
> > > > days to convert the rest of the code, and that will probably take
> > > > another 60 patches.
> > > > 
> > > > I understand that reviewing 100+ patches is going to be a pain, but
> > > > each patch currently averages about +/- 10 lines. The current
> > > > diffstat is:
> > > > 
> > > > 	37 files changed, 723 insertions(+), 722 deletions(-)
> > > > 
> > > > And that will probably double, so it's still going to be a fair
> > > > amount of change.
> > > 
> > > Is there any sort of more coarse logical breakdown of this series, or do
> > > we want/need to convert the entire codebase all at once? The individual
> > > patches sound relatively small... is there a particular method at play
> > > there? E.g., a patch per function? file? call chain?
> > 
> > I'm doing it layer by layer, starting from the linux interface
> > layers and working my way down. e.g. fs/xfs/xfs_file.c first,
> > the fs/xfs/xfs_iops.c, and so on, and there are multiple patches per
> > file for each (roughly) logical change. e.g. converting xfs_iops.c:
> [...]
> 
> I've decided that there really is too much unnecessary code churn
> from this approach. I end up converting all the call sites to negate
> the error sign, and then end up converting them back to the original
> code some time later, leaving only the source of the errors with a
> changed sign.
> 
> So, I stopped doing that to see just what the brute force, change
> source and conversions only, and I found with a few simple searches
> I could identify all the locations that need changing. So, in a
> couple of hours I churned out the patch that converted everything.
> Still pretty large, even though it only changes error values and
> conversion points.
> 
> 67 files changed, 879 insertions(+), 884 deletions(-)
> 
> Not sure how I could break this up - it really is an all-or-nothing
> patch this Big Hammer approach....
> 

Yeah, now that I look at it, it's kind of hard to review as any other
way as well. I've done some grepping and made a pass through all of the
changes. I noticed some very minor things like not all of the comments
being converted and some multi-line parameter lists going out of
alignment, but I didn't bother to even make notes of those.

I've gone through an xfstests run without any explosions as well.

A couple general observations:

- I assume the xfs_buf->b_error type change is intentional..?
- Same for the change in value for the ASSERT(error <=0 && error >=
  -1000) assert in xfs_buf_ioerror (previously it used 64k).

... and I saw a few spots that looked like could still need conversion.
A diff is inlined below.

Brian

---8<---

diff --git a/fs/xfs/xfs_attr_list.c b/fs/xfs/xfs_attr_list.c
index e76f687..62db83a 100644
--- a/fs/xfs/xfs_attr_list.c
+++ b/fs/xfs/xfs_attr_list.c
@@ -648,6 +648,6 @@ xfs_attr_list(
 	alist->al_offset[0] = context.bufsize;
 
 	error = xfs_attr_list_int(&context);
-	ASSERT(error >= 0);
+	ASSERT(error <= 0);
 	return error;
 }
diff --git a/fs/xfs/xfs_ioctl32.c b/fs/xfs/xfs_ioctl32.c
index 7207e9d..e65ea67 100644
--- a/fs/xfs/xfs_ioctl32.c
+++ b/fs/xfs/xfs_ioctl32.c
@@ -376,7 +376,7 @@ xfs_compat_attrlist_by_handle(
 		goto out_dput;
 
 	cursor = (attrlist_cursor_kern_t *)&al_hreq.pos;
-	error = -xfs_attr_list(XFS_I(dentry->d_inode), kbuf, al_hreq.buflen,
+	error = xfs_attr_list(XFS_I(dentry->d_inode), kbuf, al_hreq.buflen,
 					al_hreq.flags, cursor);
 	if (error)
 		goto out_kfree;
@@ -515,7 +515,7 @@ xfs_compat_fssetdm_by_handle(
 		goto out;
 	}
 
-	error = -xfs_set_dmattrs(XFS_I(dentry->d_inode), fsd.fsd_dmevmask,
+	error = xfs_set_dmattrs(XFS_I(dentry->d_inode), fsd.fsd_dmevmask,
 				 fsd.fsd_dmstate);
 
 out:

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

Re: [DISCUSS] Planning for new dev cycle (3.17)

[Dave Chinner]

On Thu, Jun 12, 2014 at 04:01:41PM -0400, Brian Foster wrote:
> On Wed, Jun 11, 2014 at 07:10:20PM +1000, Dave Chinner wrote:
> > On Wed, Jun 11, 2014 at 07:48:50AM +1000, Dave Chinner wrote:
> > > On Tue, Jun 10, 2014 at 07:58:57AM -0400, Brian Foster wrote:
> > > > On Tue, Jun 10, 2014 at 08:33:20AM +1000, Dave Chinner wrote:
> > > > > The issue is the negative error number patchset, and how to handle
> > > > > review and testing. The patchset is already 62 patches long and it
> > > > > converts roughly half the code base. It'll take me another couple of
> > > > > days to convert the rest of the code, and that will probably take
> > > > > another 60 patches.
> > > > > 
> > > > > I understand that reviewing 100+ patches is going to be a pain, but
> > > > > each patch currently averages about +/- 10 lines. The current
> > > > > diffstat is:
> > > > > 
> > > > > 	37 files changed, 723 insertions(+), 722 deletions(-)
> > > > > 
> > > > > And that will probably double, so it's still going to be a fair
> > > > > amount of change.
> > > > 
> > > > Is there any sort of more coarse logical breakdown of this series, or do
> > > > we want/need to convert the entire codebase all at once? The individual
> > > > patches sound relatively small... is there a particular method at play
> > > > there? E.g., a patch per function? file? call chain?
> > > 
> > > I'm doing it layer by layer, starting from the linux interface
> > > layers and working my way down. e.g. fs/xfs/xfs_file.c first,
> > > the fs/xfs/xfs_iops.c, and so on, and there are multiple patches per
> > > file for each (roughly) logical change. e.g. converting xfs_iops.c:
> > [...]
> > 
> > I've decided that there really is too much unnecessary code churn
> > from this approach. I end up converting all the call sites to negate
> > the error sign, and then end up converting them back to the original
> > code some time later, leaving only the source of the errors with a
> > changed sign.
> > 
> > So, I stopped doing that to see just what the brute force, change
> > source and conversions only, and I found with a few simple searches
> > I could identify all the locations that need changing. So, in a
> > couple of hours I churned out the patch that converted everything.
> > Still pretty large, even though it only changes error values and
> > conversion points.
> > 
> > 67 files changed, 879 insertions(+), 884 deletions(-)
> > 
> > Not sure how I could break this up - it really is an all-or-nothing
> > patch this Big Hammer approach....
> > 
> 
> Yeah, now that I look at it, it's kind of hard to review as any other
> way as well. I've done some grepping and made a pass through all of the
> changes. I noticed some very minor things like not all of the comments
> being converted and some multi-line parameter lists going out of
> alignment, but I didn't bother to even make notes of those.
> 
> I've gone through an xfstests run without any explosions as well.
> 
> A couple general observations:
> 
> - I assume the xfs_buf->b_error type change is intentional..?

yes - it was an unsigned short, which is incompaitble with negative
integer values, and there is a 2 byte hole in the xfs_buf structure
after it, anyway....

> - Same for the change in value for the ASSERT(error <=0 && error >=
>   -1000) assert in xfs_buf_ioerror (previously it used 64k).

Right - it was checking to see if it fit in an unsigned short, while
now it checks for the valid "negative errno" range the kernel uses.

> ... and I saw a few spots that looked like could still need conversion.

No surprises there...

> A diff is inlined below.

Yup, I missed a couple. I'll fold them in to the patch. Thanks!

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

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