[PATCH] xfs: reduce stack usage in xfs_page_state_convert()

[PATCH] xfs: reduce stack usage in xfs_page_state_convert()

[Denys Vlasenko]

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

Hi David,

This patch reduces xfs_page_state_convert() stack usage by 16 bytes
by eliminating some local variables, and reducing the size
of scope for other locals.

Compile tested only.

Signed-off-by: Denys Vlasenko <vda.linux@googlemail.com>

P.S.

xfs_page_state_convert() carries the following comment:
 * Calling this without startio set means we are being asked to make a dirty
 * page ready for freeing it's buffers.  When called with startio set then
 * we are coming from writepage.
which leads to the following proposal: reimplement it as two
functions, one which work as if startio parameter == 0
and the other as if startio == 1.
This will result in a bit of code duplication, but reduces
stack usage on writepage path and allows for these two functions
to have more descriptive names. (Presently the meaning of this
function needs to be explained in that comment -> function
name is not descriptive enough, because it does different things
depending on startio value).

Do you like this idea?
--
vda

[-- Attachment #2: stk2.diff --]
[-- Type: text/x-diff, Size: 2477 bytes --]

diff -urpN linux-2.6-xfs1/fs/xfs/linux-2.6/xfs_aops.c linux-2.6-xfs1.stk2/fs/xfs/linux-2.6/xfs_aops.c
--- linux-2.6-xfs1/fs/xfs/linux-2.6/xfs_aops.c	2008-04-22 04:06:46.000000000 +0200
+++ linux-2.6-xfs1.stk2/fs/xfs/linux-2.6/xfs_aops.c	2008-04-27 02:38:51.000000000 +0200
@@ -935,17 +935,11 @@ xfs_page_state_convert(
 	unsigned long           p_offset = 0;
 	unsigned int		type;
 	__uint64_t              end_offset;
-	pgoff_t                 end_index, last_index, tlast;
-	ssize_t			size, len;
+	pgoff_t                 end_index, last_index;
 	int			flags, err, iomap_valid = 0, uptodate = 1;
 	int			page_dirty, count = 0;
-	int			trylock = 0;
 	int			all_bh = unmapped;
-
-	if (startio) {
-		if (wbc->sync_mode == WB_SYNC_NONE && wbc->nonblocking)
-			trylock |= BMAPI_TRYLOCK;
-	}
+#define len (1 << inode->i_blkbits)
 
 	/* Is this page beyond the end of the file? */
 	offset = i_size_read(inode);
@@ -975,7 +969,6 @@ xfs_page_state_convert(
  	 */
 	end_offset = min_t(unsigned long long,
 			(xfs_off_t)(page->index + 1) << PAGE_CACHE_SHIFT, offset);
-	len = 1 << inode->i_blkbits;
 	p_offset = min_t(unsigned long, end_offset & (PAGE_CACHE_SIZE - 1),
 					PAGE_CACHE_SIZE);
 	p_offset = p_offset ? roundup(p_offset, len) : PAGE_CACHE_SIZE;
@@ -1031,13 +1024,18 @@ xfs_page_state_convert(
 				flags = BMAPI_WRITE | BMAPI_IGNSTATE;
 			} else if (buffer_delay(bh)) {
 				type = IOMAP_DELAY;
-				flags = BMAPI_ALLOCATE | trylock;
+				flags = BMAPI_ALLOCATE;
+				if (startio && wbc->sync_mode == WB_SYNC_NONE && wbc->nonblocking) {
+					flags = BMAPI_ALLOCATE | BMAPI_TRYLOCK;
+				}
 			} else {
 				type = IOMAP_NEW;
 				flags = BMAPI_WRITE | BMAPI_MMAP;
 			}
 
 			if (!iomap_valid) {
+				ssize_t size;
+
 				/*
 				 * if we didn't have a valid mapping then we
 				 * need to ensure that we put the new mapping
@@ -1082,6 +1080,8 @@ xfs_page_state_convert(
 			 * underneath it. Map the extent by reading it.
 			 */
 			if (!iomap_valid || flags != BMAPI_READ) {
+				ssize_t size;
+
 				flags = BMAPI_READ;
 				size = xfs_probe_cluster(inode, page, bh,
 								head, 1);
@@ -1129,6 +1129,8 @@ xfs_page_state_convert(
 		xfs_start_page_writeback(page, wbc, 1, count);
 
 	if (ioend && iomap_valid) {
+		pgoff_t tlast;
+
 		offset = (iomap.iomap_offset + iomap.iomap_bsize - 1) >>
 					PAGE_CACHE_SHIFT;
 		tlast = min_t(pgoff_t, offset, last_index);
@@ -1156,6 +1158,7 @@ error:
 		ClearPageUptodate(page);
 	}
 	return err;
+#undef len
 }
 
 /*

Re: [PATCH] xfs: reduce stack usage in xfs_page_state_convert()

[David Chinner]

On Sun, Apr 27, 2008 at 02:46:58AM +0200, Denys Vlasenko wrote:
> Hi David,
> 
> This patch reduces xfs_page_state_convert() stack usage by 16 bytes
> by eliminating some local variables, and reducing the size
> of scope for other locals.
> 
> Compile tested only.

Can you start testing your patches? if you are touching the writeback
or allocator path, there's a pretty high barrier to having patches
excepted, and testing them before is one of them. Go and download the
XFSQA suite from the xfs-cmds CVS tree on oss.sgi.com, and run your
patches through it....

> Signed-off-by: Denys Vlasenko <vda.linux@googlemail.com>
> 
> P.S.
> 
> xfs_page_state_convert() carries the following comment:
>  * Calling this without startio set means we are being asked to make a dirty
>  * page ready for freeing it's buffers.  When called with startio set then
>  * we are coming from writepage.
> which leads to the following proposal: reimplement it as two
> functions, one which work as if startio parameter == 0
> and the other as if startio == 1.
> This will result in a bit of code duplication, but reduces
> stack usage on writepage path and allows for these two functions
> to have more descriptive names. (Presently the meaning of this
> function needs to be explained in that comment -> function
> name is not descriptive enough, because it does different things
> depending on startio value).
> 
> Do you like this idea?

No. That code is complex enough with only one copy of it around. I don't
want two copies that differ subtly and hence have two different sets
of nasty, rarely hit corner cases in them.

Cheers,

Dave.
-- 
Dave Chinner
Principal Engineer
SGI Australian Software Group

Re: [PATCH] xfs: reduce stack usage in xfs_page_state_convert()

[Denys Vlasenko]

On Monday 28 April 2008 01:23, David Chinner wrote:
> > This patch reduces xfs_page_state_convert() stack usage by 16 bytes
> > by eliminating some local variables, and reducing the size
> > of scope for other locals.
> > 
> > Compile tested only.
> 
> Can you start testing your patches? if you are touching the writeback
> or allocator path, there's a pretty high barrier to having patches
> excepted, and testing them before is one of them. Go and download the

Its you who asked for patches. It's not like I decided
to nag you because I have nothing better to do. Actually,
my plate is pretty full with other things already.

> XFSQA suite from the xfs-cmds CVS tree on oss.sgi.com, and run your
> patches through it....


On Tuesday 22 April 2008 03:28, David Chinner wrote:
> Patches are welcome - I'd be over the moon if any of the known 4k
> stack advocates sent a stack reduction patch for XFS, but it seems
> that actually trying to fix the problems is much harder than
> resending a one line patch every few months....

So I went ahead and actually spend a good chunk of the week
trying to help you.

I believe that my patches were sufficiently well thought-out,
carefully implemented and reasonably tested (for a guy who
never used xfs, have no xfs partitions, and not exactly
planning to use xfs in the future).

> pretty high barrier to having patches accepted

I was honestly trying to help you. I am still willing
to do it, but at some point you have to carry some part
of a burden (maybe review and run testing?).

If you are not going to accept patches, why you are
accusing people of not sending them to you?
--
vda

Re: [PATCH] xfs: reduce stack usage in xfs_page_state_convert()

[David Chinner]

On Mon, Apr 28, 2008 at 01:48:22AM +0200, Denys Vlasenko wrote:
> On Monday 28 April 2008 01:23, David Chinner wrote:
> > > This patch reduces xfs_page_state_convert() stack usage by 16 bytes
> > > by eliminating some local variables, and reducing the size
> > > of scope for other locals.
> > > 
> > > Compile tested only.
> > 
> > Can you start testing your patches? if you are touching the writeback
> > or allocator path, there's a pretty high barrier to having patches
> > excepted, and testing them before is one of them. Go and download the
> 
> Its you who asked for patches. It's not like I decided
> to nag you because I have nothing better to do. Actually,
> my plate is pretty full with other things already.

Yes, I asked for patches. That doesn't guarantee that we'll accept
them, though, or apply criteria other than "does it compile?" when
deciding whether to accept them.

Given that an unnoticed error in the allocation and writeback paths
could result in on-disk corruption on every single XFS filesystem
that uses it, I consider asking patch submitters to do some testing
(given that we have a test suite) not at all unreasonable.

Cleanups like removing function args are trivial and we can easily
review and test them (I'm doing that for several of your patches on
ia64, x86_64 and UML), but factoring critical code is a different
level. I note that Christoph's patch to the same code included a
comment that "it passed XFSQA" - he is a long time contributor to
XFS and knows that this is critical code and has performed the due
diligence that we expect for changes to critical code.

Basically, what I'm saying is that I'm not holding you to a standard
that is different to anyone else contributing patches to XFS - it's
the same standard patches from SGI XFS engineers are held to. Yes,
it might be a higher standard than you are used to, but people make
mistakes and the processes we use attempt to prevent mistakes that
have been made in the past.

> I was honestly trying to help you. I am still willing
> to do it, but at some point you have to carry some part
> of a burden (maybe review and run testing?).

I am grateful for the time you have spent so far writing and
sending patches, and encourage you to continue doing so.

However, part of the overall burden of code changes has to be shared
by the submitter.  We're taking on the integration, QA and long term
maintenance burden by accepting your patch. A bug in a patch could
mean weeks of engineering time a year down the track when someone
finally hits the corner case that triggers the bug. That burden is
something you will never have to wear, but it's a major, major load
on us and at some times completely absorbs all our resources.

Hence to reduce the *burden on us* we ask that non-trivial patches
or patches to critical sections of code are tested first by the
submitter to help increase the initial code coverage of the patch.
It saves us all lot of pain and expense in the long run....

Cheers,

Dave.
-- 
Dave Chinner
Principal Engineer
SGI Australian Software Group

Re: [PATCH] xfs: reduce stack usage in xfs_page_state_convert()

[Christoph Hellwig]

Note that I'm happy to revisit and test patches by others if they are
useful enough.  Please add me to the CC list in that case.

Re: [PATCH] xfs: reduce stack usage in xfs_page_state_convert()

[Christoph Hellwig]

On Mon, Apr 28, 2008 at 09:23:17AM +1000, David Chinner wrote:
> No. That code is complex enough with only one copy of it around. I don't
> want two copies that differ subtly and hence have two different sets
> of nasty, rarely hit corner cases in them.

Actually the split makes some sense.  I had a ready patch to split out
releasepage which makes the whole code a lot nicer.  I didn't go forward
with it because I had this idea that it would get replaced by Chris
extent_map stuff ASAP..

Re: [PATCH] xfs: reduce stack usage in xfs_page_state_convert()

[David Chinner]

On Sun, Apr 27, 2008 at 11:50:23PM -0400, Christoph Hellwig wrote:
> On Mon, Apr 28, 2008 at 09:23:17AM +1000, David Chinner wrote:
> > No. That code is complex enough with only one copy of it around. I don't
> > want two copies that differ subtly and hence have two different sets
> > of nasty, rarely hit corner cases in them.
> 
> Actually the split makes some sense.  I had a ready patch to split out
> releasepage which makes the whole code a lot nicer.  I didn't go forward
> with it because I had this idea that it would get replaced by Chris
> extent_map stuff ASAP..

I think we need to move forward on this, Christoph - I've just found
the cause of a long standing assert failure (test 083 failing on
inode teardown with outstanding delayed allocation extents) and it
appears to me that the way XFS handles page invalidation (i.e.
->invalidate_page/->release_page) is completely broken w.r.t.
standalone calls to vmtruncate() and state being held on
buggerheads. i.e.  we're leaving delalloc turds lying around when
get_blocks returns an error in __block_prepare_write() and the write
is beyond EOF.

The problem is that block_invalidate_page() calls discard_buffer()
on every buffer head on the page, thereby stripping all the state
that xfs_vm_releasepage needs to convert delalloc extents to real
extents to avoid the assert failures that occur in different places.

Even if we had the state, xfs_vm_releasepage does the wrong thing.
We don't want to allocate those extents in this case; we want to
remove those extents because we've just truncated them away.

The unfortunate part here is that the design appears to have
carefully optimised the invalidate page path so we don't need to
call xfs_bunmapi in this case, as all normal truncates run through
xfs_setattr() and the vmtruncate call is followed by a transaction
to free all extents in the range being truncated (see the
xfs_itruncate_data() call). The only other place vmtruncate() is
called from is block_begin_write(), which assumes that de-allocation
is done by the filesystem which is not the case for XFS.

This is a bug that has been around for years. Christoph - I think
we really need to kill buffer heads as soon as possible and clean
up the ugly, ugly mess that makes up the I/O path. I'll talk
off-line with you about this....

In the mean time, I'm going to do a nasty hack that picks on
!uptodate pages with delalloc extents and xfs_free_file_space()
them and see if that works.

Cheers,

Dave.
-- 
Dave Chinner
Principal Engineer
SGI Australian Software Group