Re: [PATCH 3/3] AFS: Implement basic file write support

Andrew Morton [off-list ref] wrote:

quoted

+	BUG_ON(i_size > 0xffffffff); // TODO: use 64-bit store

You're sure this isn't user-triggerable?

Hmmm...  I'm not.  I'll whip up a patch for this.

kmap_atomic() could be used here and is better.

Yeah.  It used to have something that slept in the middle of it, but that's no
longer there.  I'll add to the patch.

We have this zero_user_page() thing heading in which could perhaps be used
here also.

Okay.  I'll have a look at it once it's there.

quoted

+	ASSERTRANGE(wb->first, <=, index, <=, wb->last);

wow.

:-)

The assertions I've put in have been very useful.

quoted

+	set_page_dirty(page);
+
+	if (PageDirty(page))
+		_debug("dirtied");
+
+	return 0;
+}

One would normally run mark_inode_dirty() after any i_size_write()?

Not in this case, I assume, because set_page_dirty() ultimately calls
__mark_inode_dirty(), but I could be wrong.

We can invalidate pages and we can truncate them and we can clean them. 
But here we have a new operation, "killing".  I wonder what that is.

I can call it invalidation if you like, though that name is already reserved
as it were:-/  I suppose it might actually make sense for me to call
invalidatepage() myself.

quoted

+	if (wbc->sync_mode != WB_SYNC_NONE)
+		wait_on_page_writeback(page);

Didn't the VFS already do that?

I'm not entirely sure.  Looking at generic_writepages(), I guess so.  I'll
patch it out.

quoted

+	if (PageWriteback(page) || !PageDirty(page)) {
+		unlock_page(page);
+		return 0;
+	}

And some of that?

Yeah.  Seems so.  I'll patch that out too.

What I'd like to do is ditch writepage() entirely - I'm not sure it's entirely
necessary with the availability of writepages() - but I'll look at that
another time.

I have this vague prehistoric memory that something can go wrong at the VFS
level if the address_space writes back more pages than it was asked to. 
But I forget what the issue was and it would be silly to have an issue
with that anyway.  Something to keep an eye out for.

Okay.

Thanks for the 'cherry-pick'.  I'll hopefully have a revision patch for you
soon.

David