On Wednesday 01 December 2004 00:20, Andrew Morton wrote:
> [email protected] wrote:
> > Fixed a file descriptor leak in the network driver when changing an IP
> > address.
> >
> > Fixed the error handling in run_helper.
> > Paolo notes:
> >
> > Actually, this is part one of the change, the exact one extracted from
> > Jeff Dike's incrementals tree before 2.6.9-rc big UML merge.
> >
> > There is some changes must be done, so I'm also sending a second patch
> > with this one, too. Separated for tracking purposes.
> >
> > Don't send this pair of ones to Linus before Jeff ACK's it - just put
> > into -mm for now.
> That makes five UML patches which I have queued up pending confirmation:
Ok, detailed answers for each one.
> hostfs-uml-set-sendfile-to-generic_file_sendfile.patch
> hostfs-uml-add-some-other-pagecache-methods.patch
For these two, I'm waiting mainline answers - the first one was tested and
worked, while the second not, but is no more intrusive. In general, the
patches themselves are good: hostfs already makes full use of the page cache.
My doubt (which actually is not related to the patches themselves) is here:
static struct address_space_operations hostfs_aops = {
.writepage = hostfs_writepage,
.readpage = hostfs_readpage,
/* .set_page_dirty = __set_page_dirty_nobuffers, */
.prepare_write = hostfs_prepare_write,
.commit_write = hostfs_commit_write
};
Actually, hostfs is a nodev filesystem, but I simply don't know if that
implies that it uses no buffers. So, should
.set_page_dirty = __set_page_dirty_nobuffers
be uncommented? Or should it be deleted (leaving it there is not a good
option).
I'm holding the patches because I don't know if using them could anyhow
trigger data loss from this bug, if it's a bug.
> uml-terminal-cleanup.patch
I don't know technically this one. It won't probably go in 2.6.10, I think
later... tested in the SuSE tree, but let's be quiet in merging _big_ things,
ok? It was also tested in a different tree, so it perfectly working on 2.6.9
does not mean perfectly working on current kernels.
Some other well tested patches (not these ones) are causing host problems,
i.e. UML processes crashing and staying in D state (this seems some kind of
ptrace bug, but still digging on this) - acked on 2.6.9 hosts.
Or dying completely but keeping some FS (a tmpfs mount used only for UML) from
being unmounted (yes, checked lsof, which shows nothing - I've heard rumors
of locks alive). It does not seem to be related to tmpfs in particular,
however.
> uml-first-part-rework-of-run_helper-and-users.patch
> uml-finish-fixing-run_helper-failure-path.patch
These are littler and somehow widely tested... but nobody complained with the
1st alone.
> Could you gents please put heads together and tell me whether and when
> these should go upstream?
--
Paolo Giarrusso, aka Blaisorblade
Linux registered user n. 292729
http://www.user-mode-linux.org/~blaisorblade
Blaisorblade <[email protected]> wrote:
>
> static struct address_space_operations hostfs_aops = {
> .writepage = hostfs_writepage,
> .readpage = hostfs_readpage,
> /* .set_page_dirty = __set_page_dirty_nobuffers, */
> .prepare_write = hostfs_prepare_write,
> .commit_write = hostfs_commit_write
> };
>
> Actually, hostfs is a nodev filesystem, but I simply don't know if that
> implies that it uses no buffers. So, should
>
> .set_page_dirty = __set_page_dirty_nobuffers
>
> be uncommented? Or should it be deleted (leaving it there is not a good
> option).
See the operation of set_page_dirty().
If you have NULL ->set_page_dirty a_op then set_page_dirty() will fall
through to __set_page_dirty_buffers().
If your fs never sets PG_private then __set_page_dirty_buffers() will just
do what __set_page_dirty_nobuffers() does.
Without having looked at it, I'm sure that hostfs does not use
buffer_heads. So setting your ->set_page_dirty a_op to point at
__set_page_dirty_nobuffers() is a reasonable thing to do - it'll provide a
slight speedup.
On Wednesday 01 December 2004 01:33, Andrew Morton wrote:
> Blaisorblade <[email protected]> wrote:
> > static struct address_space_operations hostfs_aops = {
> > .writepage = hostfs_writepage,
> > .readpage = hostfs_readpage,
> > /* .set_page_dirty = __set_page_dirty_nobuffers, */
> > .prepare_write = hostfs_prepare_write,
> > .commit_write = hostfs_commit_write
> > };
> >
> > Actually, hostfs is a nodev filesystem, but I simply don't know if that
> > implies that it uses no buffers. So, should
> >
> > .set_page_dirty = __set_page_dirty_nobuffers
> >
> > be uncommented? Or should it be deleted (leaving it there is not a good
> > option).
>
> See the operation of set_page_dirty().
> If you have NULL ->set_page_dirty a_op then set_page_dirty() will fall
> through to __set_page_dirty_buffers().
Yes, I already understood this, the easy part.
> If your fs never sets PG_private then __set_page_dirty_buffers() will just
> do what __set_page_dirty_nobuffers() does.
Ok, I didn't imagine this (looks reasonable though).
Apart the fact that the "race with truncate" check is a bit different: this is
is in __set_page_dirty_nobuffers(mm/page-writeback.c) and probably wants
being added to the _buffers version, since it does cannot do anything else
than triggering a BUG (which you don't see currently, I guess):
[...]
mapping2 = page_mapping(page);
if (mapping2) { /* Race with truncate? */
BUG_ON(mapping2 != mapping);
[...]
> Without having looked at it, I'm sure that hostfs does not use
> buffer_heads.
It can compile without
#include <linux/buffer_head.h>
(even if the include is there), and it never seem to set any page as buffer
(by setting the PG_private bit, which can have other meanings too I guess in
other contexts).
So I guessed this right the first time - I was not sure if it was so
straightforward.
> So setting your ->set_page_dirty a_op to point at
> __set_page_dirty_nobuffers() is a reasonable thing to do - it'll provide a
> slight speedup.
If it is a speedup only, then I'm happier - I was especially worried if it was
going to create possible bugs, even because there are someone has reported
problems in listing large folders... never reproduced it here and most users
don't see it, so not yet any clues.
Thanks a lot for the help!
--
Paolo Giarrusso, aka Blaisorblade
Linux registered user n. 292729
http://www.user-mode-linux.org/~blaisorblade