hundreds of mount --bind mountpoints?
Subsystems (4)
Hi,
I'm still working on a packaging system for diskless (quasi-embedded) devices. The root filesystem is all tmpfs, and I attach packages inside it. Since symlinks in a tmpfs filesystem cost 4k each (ouch!), I'm considering using mount --bind for everything. This appears to use very little memory, but I'm wondering if I'll run into problems when I start having many hundreds of bind mountings. Any feel for this?
regards,
David
--
David L. Parsley
Roanoke College Network Administrator
Quoted 8 lines · David L. Parsley
Hi,
I'm still working on a packaging system for diskless (quasi-embedded) devices. The root filesystem is all tmpfs, and I attach packages inside it. Since symlinks in a tmpfs filesystem cost 4k each (ouch!), I'm considering using mount --bind for everything. This appears to use very little memory, but I'm wondering if I'll run into problems when I start having many hundreds of bind mountings. Any feel for this?
Memory use is sizeof(struct vfsmount) per binding. In principle, you can get in trouble when size of /proc/mount will get past 4Kb - you'll get only first 4 (actually 3, IIRC) kilobytes, so stuff that relies on the contents of said file may get unhappy. It's fixable, though.
Hi David,
Quoted 5 lines · David L. Parsley
I'm still working on a packaging system for diskless (quasi-embedded) devices. The root filesystem is all tmpfs, and I attach packages inside it. Since symlinks in a tmpfs filesystem cost 4k each (ouch!), I'm considering using mount --bind for everything.
What about fixing tmpfs instead?
Greetings
Christoph
Quoted 6 lines · Christoph Rohland
Quoted 3 lines · David L. Parsley
attach packages inside it. Since symlinks in a tmpfs filesystem cost 4k each (ouch!), I'm considering using mount --bind for everything.
What about fixing tmpfs instead?
The question is: How? If you do it like ramfs, you cannot swap these symlinks and this is effectively a mlock(symlink) operation allowed for normal users. -> BAD!
One idea is to only use a page, if the entry will be pushed into swap and thus only wasting swap, not memory (where we have more of it).
But allocating a page on memory pressure is also not a bright idea.
OTOH we could force this entry to swap immedately, after we copied it from the dentry. So we can do an GFP_ATOMIC allocation and do not too much harm to memory pressure and only make the IO a bit stormier.
I think there are a lot of races, which I don't see now.
So please don't beat me too much, if this is a completly stupid idea, ok? ;-)
Regards
Ingo Oeser
Signature · 2 lines
10.+11.03.2001 - 3. Chemnitzer LinuxTag <http://www.tu-chemnitz.de/linux/tag>
<<<<<<<<<<<< been there and had much fun >>>>>>>>>>>>Quoted 11 lines · Christoph Rohland
Hi David,
Quoted 5 lines · David L. Parsley
I'm still working on a packaging system for diskless (quasi-embedded) devices. The root filesystem is all tmpfs, and I attach packages inside it. Since symlinks in a tmpfs filesystem cost 4k each (ouch!), I'm considering using mount --bind for everything.
What about fixing tmpfs instead?
That would be great - are you volunteering? ;-) Seriously - I might be able to look at what ramfs does and port that to tmpfs for my needs, but that's about the extent of my kernel hacking skills. For now, mount --bind looks like it'll work just fine. If somebody wants to fix tmpfs, I'll be happy to test patches; it'll just change a couple of lines in my package loading logic (mount --bind x y -> ln -s x y).
What I'm not sure of is which solution is actually 'better' - I'm guessing that performance-wise, neither will make a noticable difference, so I guess memory usage would be the deciding factor. If I can get a lot closer to the size of a symlink (10-20 bytes) that would be best. The issue with /proc/mounts really shouldn't hurt anything - I could almost get by without mounting /proc anyway, it's mainly a convenience.
regards,
David
Signature · 3 lines
David L. Parsley Network Administrator Roanoke College
Quoted 3 lines · David L. Parsley
What I'm not sure of is which solution is actually 'better' - I'm guessing that performance-wise, neither will make a noticable difference, so I guess memory usage would be the deciding factor. If I
Bindings are faster on lookup. For obvious reasons - in case of symlinks you do name resolution every time you traverse the link; in case of bindings it is done when you create them.
Hi Ingo,
Quoted 11 lines · Ingo Oeser
Quoted 6 lines · Christoph Rohland
Quoted 3 lines · David L. Parsley
attach packages inside it. Since symlinks in a tmpfs filesystem cost 4k each (ouch!), I'm considering using mount --bind for everything.
What about fixing tmpfs instead?
The question is: How? If you do it like ramfs, you cannot swap these symlinks and this is effectively a mlock(symlink) operation allowed for normal users. -> BAD!
How about storing it into the inode structure if it fits into the fs-private union? If it is too big we allocate the page as we do it now. The union has 192 bytes. This should be sufficient for most cases.
Greetings
Christoph
Hi Chris,
Quoted 8 lines · Christoph Rohland
Quoted 3 lines
The question is: How? If you do it like ramfs, you cannot swap these symlinks and this is effectively a mlock(symlink) operation allowed for normal users. -> BAD!
How about storing it into the inode structure if it fits into the fs-private union? If it is too big we allocate the page as we do it now. The union has 192 bytes. This should be sufficient for most cases.
Great idea. We allocate this space anyway. And we don't have to care about the internals of this union, because never have to use it outside the kernel ;-)
I like it. ext2fs does the same, so there should be no VFS hassles involved. Al?
Regards
Ingo Oeser
Signature · 2 lines
10.+11.03.2001 - 3. Chemnitzer LinuxTag <http://www.tu-chemnitz.de/linux/tag>
<<<<<<<<<<<< been there and had much fun >>>>>>>>>>>>Quoted 12 lines · Ingo Oeser
Hi Chris,
Quoted 8 lines · Christoph Rohland
Quoted 3 lines
The question is: How? If you do it like ramfs, you cannot swap these symlinks and this is effectively a mlock(symlink) operation allowed for normal users. -> BAD!
How about storing it into the inode structure if it fits into the fs-private union? If it is too big we allocate the page as we do it now. The union has 192 bytes. This should be sufficient for most cases.
Quoted 6 lines
Great idea. We allocate this space anyway. And we don't have to care about the internals of this union, because never have to use it outside the kernel ;-)
I like it. ext2fs does the same, so there should be no VFS hassles involved. Al?
We should get ext2 and friends to move the sucker _out_ of struct inode. As it is, sizeof(struct inode) is way too large. This is 2.5 stuff, but it really has to be done. More filesystems adding stuff into the union is a Bad Thing(tm). If you want to allocates space - allocate if yourself; ->clear_inode() is the right place for freeing it.
Quoted 4 lines · Alexander Viro
Quoted 4 lines
Great idea. We allocate this space anyway. And we don't have to care about the internals of this union, because never have to use it outside the kernel ;-)
Quoted 8 lines
Quoted 2 lines
I like it. ext2fs does the same, so there should be no VFS hassles involved. Al?
We should get ext2 and friends to move the sucker _out_ of struct inode. As it is, sizeof(struct inode) is way too large. This is 2.5 stuff, but it really has to be done. More filesystems adding stuff into the union is a Bad Thing(tm). If you want to allocates space - allocate if yourself; ->clear_inode() is the right place for freeing it.
You need an inode anyway. So why not using the space in it? tmpfs would only use sizeof(*inode.u)-sizeof(struct shmem_inode_info) for this kind of symlinks.
Last time we suggested this, people ended up with some OS trying it and getting worse performance.
Why? You need to allocate the VFS-inode (vnode in other OSs) and the on-disk-inode anyway at the same time. You get better performance and less fragmentation, if you allocate them both together[1].
So that struct inode around is ok.
BTW: Is it still less than one page? Then it doesn't make me
nervous. Why? Guess what granularity we allocate at, if we just store pointers instead of the inode.u. Or do you like every FS creating his own slab cache?
Regards
Ingo Oeser
[1] Which is true for other allocations, too.
Signature · 2 lines
10.+11.03.2001 - 3. Chemnitzer LinuxTag <http://www.tu-chemnitz.de/linux/tag>
<<<<<<<<<<<< been there and had much fun >>>>>>>>>>>>Quoted 2 lines · In article <[email protected]> you wrote:
Last time we suggested this, people ended up with some OS trying it and getting worse performance.
Which OS? Neither BSD nor SVR4/SVR5 (or even SVR3) do that.
Quoted 4 lines
Why? You need to allocate the VFS-inode (vnode in other OSs) and the on-disk-inode anyway at the same time. You get better performance and less fragmentation, if you allocate them both together[1].
Because having an union in generic code that includes filesystem-specific memebers is ugly? It's one of those a little more performance for a lot of bad style optimizations.
Christoph
Signature · 2 lines
Of course it doesn't work. We've performed a software upgrade. Whip me. Beat me. Make me maintain AIX.
Quoted 18 lines · Ingo Oeser
Quoted 4 lines · Alexander Viro
Quoted 4 lines
Great idea. We allocate this space anyway. And we don't have to care about the internals of this union, because never have to use it outside the kernel ;-)
Quoted 8 lines
Quoted 2 lines
I like it. ext2fs does the same, so there should be no VFS hassles involved. Al?
We should get ext2 and friends to move the sucker _out_ of struct inode. As it is, sizeof(struct inode) is way too large. This is 2.5 stuff, but it really has to be done. More filesystems adding stuff into the union is a Bad Thing(tm). If you want to allocates space - allocate if yourself; ->clear_inode() is the right place for freeing it.
You need an inode anyway. So why not using the space in it? tmpfs would only use sizeof(*inode.u)-sizeof(struct
shmem_inode_info) for this kind of symlinks.
Quoted 7 lines
Last time we suggested this, people ended up with some OS trying it and getting worse performance.
Why? You need to allocate the VFS-inode (vnode in other OSs) and the on-disk-inode anyway at the same time. You get better performance and less fragmentation, if you allocate them both together[1].
We want to take out that union because it sucks for virtual filesystems. Besides, it's ugly.
Regards, Richard.... Permanent: [email protected] Current: [email protected]
Quoted 5 lines · Christoph Hellwig
Quoted 2 lines · In article <[email protected]> you wrote:
Last time we suggested this, people ended up with some OS trying it and getting worse performance.
Which OS? Neither BSD nor SVR4/SVR5 (or even SVR3) do that.
Don't remember. I think Larry McVoy told the story, so I cc'ed him ;-)
Quoted 3 lines
Because having an union in generic code that includes filesystem-specific memebers is ugly? It's one of those a little more performance for a lot of bad style optimizations.
We have this kind of stuff all over the place. If we allocate some small amount of memory and and need some small amount associated with this memory, there is no problem with a little waste.
Waste is better than fragmentation. This is the lesson people learned from segments in the ia32.
Objects are easier to manage, if they are the same size.
Regards
Ingo Oeser
Signature · 2 lines
10.+11.03.2001 - 3. Chemnitzer LinuxTag <http://www.tu-chemnitz.de/linux/tag>
<<<<<<<<<<<< been there and had much fun >>>>>>>>>>>>Quoted 4 lines · Ingo Oeser
We have this kind of stuff all over the place. If we allocate some small amount of memory and and need some small amount associated with this memory, there is no problem with a little waste.
Little? How about quarter of kilobyte per inode? sizeof(struct inode) is nearly half-kilobyte. And icache can easily get to ~100000 elements.
Quoted 4 lines
Waste is better than fragmentation. This is the lesson people learned from segments in the ia32.
Objects are easier to manage, if they are the same size.
So don't keep them in the same cache. Notice that quite a few systems keep vnode separately from fs-specific data. For a very good reason.
Al
Quoted 2 lines · Richard Gooch
We want to take out that union because it sucks for virtual filesystems. Besides, it's ugly.
I hope you won't mind if people trash this with benchmarks.
Quoted 6 lines · Albert D. Cahalan
Quoted 2 lines · Richard Gooch
We want to take out that union because it sucks for virtual filesystems. Besides, it's ugly.
I hope you won't mind if people trash this with benchmarks.
But they can't. At least, not for a well designed patch. If there is a real issue of fragmentation, then there are ways to fix that without using a bloated union structure. Don't punish some filesystems just because others have a problem.
Solutions to avoid fragmentation:
- keep a separate VFSinode and FSinode slab cache
- allocate an enlarged VFSinode that contains the FSinode at the end, with the generic pointer in the VFSinode part pointing to FSinode part.
It's simply wrong to bloat everyone because some random FS found it easier to thow in a union.
Besides, for every benchmark that shows how fragmentation hurts, I can generate a benchmark showing how inode bloat hurts. Lies, damn lies and benchmarks.
Regards, Richard.... Permanent: [email protected] Current: [email protected]
Quoted 10 lines · Ingo Oeser
Quoted 5 lines
We should get ext2 and friends to move the sucker _out_ of struct inode. As it is, sizeof(struct inode) is way too large. This is 2.5 stuff, but it really has to be done. More filesystems adding stuff into the union is a Bad Thing(tm). If you want to allocates space - allocate if yourself; ->clear_inode() is the right place for freeing it.
BTW: Is it still less than one page? Then it doesn't make me
nervous. Why? Guess what granularity we allocate at, if we just store pointers instead of the inode.u. Or do you like every FS creating his own slab cache?
I would much rather we allocate a slab cache for each fs type (it would be trivial at register_fs time). Most people have only a limited number of filesystems active at a single time, yet tens or hundreds of thousands of inodes in the inode slab cache. Making the per-fs private inode data as small as possible would reduce memory wastage considerably, and not impact performance (AFAICS) if we use a per-fs type slab cache for fs private data.
Consider, when I was doing some fs benchmark, my inode slab cache was over 120k items on a 128MB machine. At 480 butes per inode, this is almost 58 MB, close to half of RAM. Reducing this to exactly ext2 sized inodes would save (50 - 27) * 4 * 120k = 11MB of memory (on 32-bit systems)!!! (This assumes nfs_inode_info is the largest).
This also makes it possible to safely (and efficiently) use external filesystem modules without the need to recompile the kernel. Granted, if the external filesystem doesn't use more than the largest .u struct, then it is currently possible as well, but that number changes, so it is not safe.
Cheers, Andreas
Signature · 3 lines
Andreas Dilger \ "If a man ate a pound of pasta and a pound of antipasto,
\ would they cancel out, leaving him still hungry?"
http://www-mddsp.enel.ucalgary.ca/People/adilger/ -- DogbertQuoted 1 line · Richard Gooch
- keep a separate VFSinode and FSinode slab cache
Yup.
Quoted 3 lines
- allocate an enlarged VFSinode that contains the FSinode at the end, with the generic pointer in the VFSinode part pointing to FSinode part.
Please, don't. It would help with bloat only if you allocated these beasts separately for each fs and then you end up with _many_ allocators that can generate pointer to struct inode.
"One type - one allocator" is a good rule - violating it turns into major PITA couple of years down the road 9 times out of 10.
Quoted 12 lines · Alexander Viro
Quoted 1 line · Richard Gooch
- keep a separate VFSinode and FSinode slab cache
Yup.
Quoted 3 lines
- allocate an enlarged VFSinode that contains the FSinode at the end, with the generic pointer in the VFSinode part pointing to FSinode part.
Quoted 6 lines
Please, don't. It would help with bloat only if you allocated these beasts separately for each fs and then you end up with _many_ allocators that can generate pointer to struct inode.
"One type - one allocator" is a good rule - violating it turns into major PITA couple of years down the road 9 times out of 10.
Agreed. The better option is the separate VFSinode and FSinode caches. The enlarged inode scheme is also ugly, like the unions. It's just less bloated :-)
Regards, Richard.... Permanent: [email protected] Current: [email protected]
Quoted 5 lines · Alexander Viro
Quoted 1 line · Richard Gooch
- keep a separate VFSinode and FSinode slab cache
Yup.
Would it make sense to unify these with the struct address_space ?
regards,
Rik
--
Linux MM bugzilla: http://linux-mm.org/bugzilla.shtml
Virtual memory is like a game you can't win; However, without VM there's truly nothing to lose...
http://www.surriel.com/
Quoted 8 lines · Ingo Oeser
Last time we suggested this, people ended up with some OS trying it and getting worse performance.
Why? You need to allocate the VFS-inode (vnode in other OSs) and the on-disk-inode anyway at the same time. You get better performance and less fragmentation, if you allocate them both together[1].
Quoted 6 lines
So that struct inode around is ok.
BTW: Is it still less than one page? Then it doesn't make me
nervous. Why? Guess what granularity we allocate at, if we just store pointers instead of the inode.u. Or do you like every FS creating his own slab cache?
I've actually got the coda_inode_info (inode->u.u_coda_fs_i) split out of the union in my development kernel. It doesn't shrink the size of the struct inode yet, Coda isn't the biggest user at the moment.
But, it forced me to do several cleanups in the code, and it even has resulted in fixing a 'leak'. Not a real memory loss leak one, but we left uninitialized inodes around in the icache for no good reason. Also changing a but in a coda specific header file does trigger an almost complete rebuild of the whole kernel (coda.h -> coda_fs_i.h -> fs.h -> everything?)
The allocation overhead really isn't that bad. kmalloc/kfree are also using the slabcache, and a trivial variant using a 'private' slabcache gave me the counters to find the 'leak' I mentioned before.
I can't really evaluate performance impacts. The struct inode is still the same size, so for now there even is a little bit of additional memory pressure. Also, Coda wasn't really developed to achieve high performance but more to explore novel features.
Jan
Quoted 6 lines · Jan Harkes
Quoted 4 lines · Ingo Oeser
BTW: Is it still less than one page? Then it doesn't make me
nervous. Why? Guess what granularity we allocate at, if we just store pointers instead of the inode.u. Or do you like every FS creating his own slab cache?
Oh, for crying out loud. All it takes is half an hour per filesystem. Here - completely untested patch that does it for NFS. Took about that long. Absolutely straightforward, very easy to verify correctness.
Some stuff may need tweaking, but not much (e.g. some functions should take nfs_inode_info instead of inodes, etc.). From the look of flushd cache it seems that we would be better off with cyclic lists instead of single-linked ones for the hash, but I didn't look deep enough.
So consider the patch below as proof-of-concept. Enjoy:
fs/nfs/flushd.c +6 -6
@@ -162,11 +162,11 @@if (NFS_FLAGS (inode) &NFS_INO_FLUSH ) goto out; - inode->u.nfs_i.hash_next =NULL ; +NFS_I (inode)->hash_next =NULL ; q = &cache->inodes;while (*q) - q = &(*q)->u.nfs_i.hash_next; + q = &NFS_I (*q)->hash_next; *q = inode;/* Note: we increase the inode i_count in order to prevent
@@ -188,9 +188,9 @@ q = &cache->inodes;while (*q && *q != inode) - q = &(*q)->u.nfs_i.hash_next; + q = &NFS_I (*q)->hash_next;if (*q) { - *q = inode->u.nfs_i.hash_next; + *q =NFS_I (inode)->hash_next;NFS_FLAGS (inode) &= ~NFS_INO_FLUSH ;iput (inode); }
@@ -238,8 +238,8 @@ cache->inodes =NULL ;while ((inode = next) !=NULL ) { - next = next->u.nfs_i.hash_next; - inode->u.nfs_i.hash_next =NULL ; + next =NFS_I (next)->hash_next; +NFS_I (inode)->hash_next =NULL ;NFS_FLAGS (inode) &= ~NFS_INO_FLUSH ;if (flush) {
fs/nfs/inode.c +59 -11
138 lines not on this page · raw message
fs/nfs/read.c +13 -10
@@ -153,7 +153,7 @@ {struct list_head *head, *next; - head = &inode->u.nfs_i.read; + head = &NFS_I (inode)->read; next = head->next;while (next != head) {struct nfs_page *req =nfs_list_entry (next);
@@ -183,11 +183,12 @@ nfs_mark_request_read(struct nfs_page *req) {struct inode *inode = req->wb_inode; +struct nfs_inode_info *nfsi =NFS_I (inode);spin_lock (&nfs_wreq_lock);if (list_empty (&req->wb_list)) { -nfs_list_add_request (req, &inode->u.nfs_i.read); - inode->u.nfs_i.nread++; +nfs_list_add_request (req, &nfsi->read); + nfsi->nread++; }spin_unlock (&nfs_wreq_lock);/*
@@ -234,7 +235,7 @@break ; } -if (inode->u.nfs_i.nread >=NFS_SERVER (inode)->rpages || +if (NFS_I (inode)->nread >=NFS_SERVER (inode)->rpages ||page_index (page) == (inode->i_size +PAGE_CACHE_SIZE -1 ) >>PAGE_CACHE_SHIFT )nfs_pagein_inode (inode,0 ,0 );if (new)
@@ -372,10 +373,11 @@ nfs_scan_read_timeout(struct inode *inode, struct list_head *dst) { int pages; +struct nfs_inode_info *nfsi =NFS_I (inode);spin_lock (&nfs_wreq_lock); - pages =nfs_scan_list_timeout (&inode->u.nfs_i.read, dst, inode); - inode->u.nfs_i.nread -= pages; -if ((inode->u.nfs_i.nread ==0 ) !=list_empty (&inode->u.nfs_i.read)) + pages =nfs_scan_list_timeout (&nfsi->read, dst, inode); + nfsi->nread -= pages; +if ((nfsi->nread ==0 ) !=list_empty (&nfsi->read))printk (KERN_ERR "NFS: desynchronized value of nfs_i.nread.\n" );spin_unlock (&nfs_wreq_lock);return pages;
@@ -385,10 +387,11 @@ nfs_scan_read(struct inode *inode, struct list_head *dst, unsigned long idx_start, unsigned int npages) { int res; +struct nfs_inode_info *nfsi =NFS_I (inode);spin_lock (&nfs_wreq_lock); - res =nfs_scan_list (&inode->u.nfs_i.read, dst,NULL , idx_start, npages); - inode->u.nfs_i.nread -= res; -if ((inode->u.nfs_i.nread ==0 ) !=list_empty (&inode->u.nfs_i.read)) + res =nfs_scan_list (&nfsi->read, dst,NULL , idx_start, npages); + nfsi->nread -= res; +if ((nfsi->nread ==0 ) !=list_empty (&nfsi->read))printk (KERN_ERR "NFS: desynchronized value of nfs_i.nread.\n" );spin_unlock (&nfs_wreq_lock);return res;
fs/nfs/write.c +40 -30
185 lines not on this page · raw message
include/linux/fs.h +0 -1
@@ -448,7 +448,6 @@struct msdos_inode_info msdos_i;struct umsdos_inode_info umsdos_i;struct iso_inode_info isofs_i; -struct nfs_inode_info nfs_i;struct sysv_inode_info sysv_i;struct affs_inode_info affs_i;struct ufs_inode_info ufs_i;
include/linux/nfs_fs.h +20 -15
@@ -63,39 +63,44 @@*/ #define NFS_SUPER_MAGIC 0x6969 -#define NFS_FH (inode) (&(inode)->u.nfs_i.fh) +static inline struct nfs_inode_info *NFS_I (struct inode *inode) +{ +return (struct nfs_inode_info *)inode->u.generic_ip; +} + +#define NFS_FH (inode) (&NFS_I(inode)->fh)#define NFS_SERVER (inode) (&(inode)->i_sb->u.nfs_sb.s_server)#define NFS_CLIENT (inode) (NFS_SERVER(inode)->client)#define NFS_PROTO (inode) (NFS_SERVER(inode)->rpc_ops)#define NFS_REQUESTLIST (inode) (NFS_SERVER(inode)->rw_requests)#define NFS_ADDR (inode) (RPC_PEERADDR(NFS_CLIENT(inode)))#define NFS_CONGESTED (inode) (RPC_CONGESTED(NFS_CLIENT(inode))) -#define NFS_COOKIEVERF (inode) ((inode)->u.nfs_i.cookieverf) -#define NFS_READTIME (inode) ((inode)->u.nfs_i.read_cache_jiffies) -#define NFS_CACHE_CTIME (inode) ((inode)->u.nfs_i.read_cache_ctime) -#define NFS_CACHE_MTIME (inode) ((inode)->u.nfs_i.read_cache_mtime) -#define NFS_CACHE_ATIME (inode) ((inode)->u.nfs_i.read_cache_atime) -#define NFS_CACHE_ISIZE (inode) ((inode)->u.nfs_i.read_cache_isize) -#define NFS_NEXTSCAN (inode) ((inode)->u.nfs_i.nextscan) +#define NFS_COOKIEVERF (inode) (NFS_I(inode)->cookieverf) +#define NFS_READTIME (inode) (NFS_I(inode)->read_cache_jiffies) +#define NFS_CACHE_CTIME (inode) (NFS_I(inode)->read_cache_ctime) +#define NFS_CACHE_MTIME (inode) (NFS_I(inode)->read_cache_mtime) +#define NFS_CACHE_ATIME (inode) (NFS_I(inode)->read_cache_atime) +#define NFS_CACHE_ISIZE (inode) (NFS_I(inode)->read_cache_isize) +#define NFS_NEXTSCAN (inode) (NFS_I(inode)->nextscan)#define NFS_CACHEINV (inode) \ do { \ NFS_READTIME(inode) = jiffies - NFS_MAXATTRTIMEO(inode) - 1; \ } while (0) -#define NFS_ATTRTIMEO (inode) ((inode)->u.nfs_i.attrtimeo) +#define NFS_ATTRTIMEO (inode) (NFS_I(inode)->attrtimeo)#define NFS_MINATTRTIMEO (inode) \ (S_ISDIR(inode->i_mode)? NFS_SERVER(inode)->acdirmin \ : NFS_SERVER(inode)->acregmin)#define NFS_MAXATTRTIMEO (inode) \ (S_ISDIR(inode->i_mode)? NFS_SERVER(inode)->acdirmax \ : NFS_SERVER(inode)->acregmax) -#define NFS_ATTRTIMEO_UPDATE (inode) ((inode)->u.nfs_i.attrtimeo_timestamp) +#define NFS_ATTRTIMEO_UPDATE (inode) (NFS_I(inode)->attrtimeo_timestamp) -#define NFS_FLAGS (inode) ((inode)->u.nfs_i.flags) +#define NFS_FLAGS (inode) (NFS_I(inode)->flags)#define NFS_REVALIDATING (inode) (NFS_FLAGS(inode) & NFS_INO_REVALIDATING)#define NFS_STALE (inode) (NFS_FLAGS(inode) & NFS_INO_STALE) -#define NFS_FILEID (inode) ((inode)->u.nfs_i.fileid) -#define NFS_FSID (inode) ((inode)->u.nfs_i.fsid) +#define NFS_FILEID (inode) (NFS_I(inode)->fileid) +#define NFS_FSID (inode) (NFS_I(inode)->fsid)/* Inode Flags */ #define NFS_USE_READDIRPLUS (inode) ((NFS_FLAGS(inode) & NFS_INO_ADVISE_RDPLUS) ? 1 : 0)
@@ -212,13 +217,13 @@static inline int nfs_have_read (struct inode *inode) { -return !list_empty (&inode->u.nfs_i.read); +return !list_empty (&NFS_I (inode)->read); }static inline int nfs_have_writebacks (struct inode *inode) { -return !list_empty (&inode->u.nfs_i.writeback); +return !list_empty (&NFS_I (inode)->writeback); } static inline int
Hi Alexander,
Quoted 9 lines · Alexander Viro
Quoted 2 lines
I like it. ext2fs does the same, so there should be no VFS hassles involved. Al?
We should get ext2 and friends to move the sucker _out_ of struct inode. As it is, sizeof(struct inode) is way too large. This is 2.5 stuff, but it really has to be done. More filesystems adding stuff into the union is a Bad Thing(tm). If you want to allocates space - allocate if yourself; ->clear_inode() is the right place for freeing it.
Yes, I agree that the union is way too large and I did not plan to extend it but simply use the size it has.
if (strlen(path) < sizeof(inode->u))
inline the symlink;
else
put it into the page cache;So if somebody really cleans up the private inode structures it will not trigger that often any more and we perhaps have to rethink the idea.
But also if we use struct shmem_inode_info which is 92 bytes right now we would inline all symlinks on my machine.
If we reduced its size to 32 (which could be easily done) we would still inline 6642 out of 9317 symlinks on my machine. That's not bad.
Greetings
Christoph
Quoted 1 line · [email protected] said:
Oh, for crying out loud. All it takes is half an hour per filesystem.
Half an hour? If it takes more than about 5 minutes for JFFS2 I'd be very surprised.
--
dwmw2
Quoted 6 lines · David Woodhouse
Quoted 1 line · [email protected] said:
Oh, for crying out loud. All it takes is half an hour per filesystem.
Half an hour? If it takes more than about 5 minutes for JFFS2 I'd be very surprised.
<tone polite> What's stopping you? </tone>
You _are_ JFFS maintainer, aren't you?
Quoted 2 lines · [email protected] said:
<tone polite> What's stopping you? </tone> You _are_ JFFS maintainer,
aren't you?
It already uses...
#define JFFS2_INODE_INFO(i) (&i->u.jffs2_i)
It's trivial to switch over when the size of the inode union goes below the size of struct jffs2_inode_info. Until then, I'd just be wasting space.
--
dwmw2
Hi Al,
Quoted 5 lines · Alexander Viro
Quoted 2 lines
Half an hour? If it takes more than about 5 minutes for JFFS2 I'd be very surprised.
<tone polite> What's stopping you? </tone>
You _are_ JFFS maintainer, aren't you?
So is this the start to change all filesystems in 2.4? I am not sure we should do that.
Greetings
Christoph
Quoted 11 lines · Christoph Rohland
Hi Al,
Quoted 5 lines · Alexander Viro
Quoted 2 lines
Half an hour? If it takes more than about 5 minutes for JFFS2 I'd be very surprised.
<tone polite> What's stopping you? </tone>
You _are_ JFFS maintainer, aren't you?So is this the start to change all filesystems in 2.4? I am not sure we should do that.
Encapsulation part is definitely worth doing - it cleans the code up and doesn't change the result of compile. Adding allocation/freeing/ cache initialization/cache removal and chaninging FOOFS_I() definition - well, it's probably worth to keep such patches around, but whether to switch any individual filesystem during 2.4 is a policy decision. Up to maintainer, indeed. Notice that these patches (separate allocation per se) are going to be within 3-4Kb per filesystem _and_ completely straightforward.
What I would like to avoid is scenario like
Maintainers of filesystems with large private inodes: Why would we separate them? We would only waste memory, since the other filesystems stay in ->u and keep it large.
Maintainers of the rest of filesystems: Since there's no patches that would take large stuff out of ->u, why would we bother?
So yes, IMO having such patches available _is_ a good thing. And in 2.5 we definitely want them in the tree. If encapsulation part gets there during 2.4 and separate allocation is available for all of them it will be easier to do without PITA in process.
Al
Quoted 5 lines · [email protected] said:
What I would like to avoid is scenario like
Maintainers of filesystems with large private inodes: Why would we separate them? We would only waste memory, since the other filesystems stay in ->u and keep it large.
Quoted 7 lines
Maintainers of the rest of filesystems: Since there's no patches that would take large stuff out of ->u, why would we bother?
So yes, IMO having such patches available _is_ a good thing. And in 2.5 we definitely want them in the tree. If encapsulation part gets there during 2.4 and separate allocation is available for all of them it will be easier to do without PITA in process.
JFFS2 has the encapsulation part already. I'll make it do separate allocation in 2.5, when it's actually a gain.
--
dwmw2
Hi Al,
Quoted 4 lines · Alexander Viro
So yes, IMO having such patches available _is_ a good thing. And in 2.5 we definitely want them in the tree. If encapsulation part gets there during 2.4 and separate allocation is available for all of them it will be easier to do without PITA in process.
OK I will do that for tmpfs soon. And I will do the symlink inlining with that patch.
Greetings
Christoph
Can't read SCSI TAPE
In reply to Alexander ViroDear sirs,
Although 'tar' can write to SCSI-TAPE, can't read from. 'tar' reports ....
......
-rw-r--r-- root/root xxxxx 2001-xx-xx 01:23 usr/bin/xxxxxx
tar: Skipping to next file header <------"A"
-rw-r--r-- root/root xxxxx 2001-xx-xx 01:23 usr/bin/xxxxxxx
......
"A" means written data is wrong, doesn't it???
Thanks for any help.
------------------------------------------
Detailed ->
System...
Kernel : 2.2.11 + raid0145-19990824-2.2.11.gz
or
2.2.11tar : GNU tar 1.12
mt : mt-st v. 0.4
glibc2 : glibc-2.0.7pre6
Hardware...
Mother : Intel Celeron x2 (SMP)
TAPE drv : SONY SDT-9000
TAPE : DDS1 DDS2 DDS3
SCSI card: AHA-1542
Cable : SCSI-2 Hi-impeadance , length 0.5m
------------------------------------------
Signature · 1 line
Masaki Tsuji
Quoted 3 lines · Christoph Rohland
OK I will do that for tmpfs soon. And I will do the symlink inlining with that patch.
Wow, this thread really exploded, eh? But thanks, Christoph, I look forward to seeing
your patch. 4k symlinks really suck for embedders who never swap out pages. ;-)
regards,
David
Signature · 3 lines
David L. Parsley Network Administrator Roanoke College
Quoted 8 lines · Al
Encapsulation part is definitely worth doing - it cleans the code up and doesn't change the result of compile. Adding allocation/freeing/ cache initialization/cache removal and chaninging
FOOFS_I()definition - well, it's probably worth to keep such patches around, but whether to switch any individual filesystem during 2.4 is a policy decision. Up to maintainer, indeed. Notice that these patches (separate allocation per se) are going to be within 3-4Kb per filesystem _and_ completely straightforward.
One thing to watch out for is that the current code zeros the u. struct for us (as you pointed out to me previously), but allocating from the slab cache will not... This could be an interesting source of bugs for some filesystems that assume zero'd inode_info structs.
Fortunately, it doesn't appear that my patch to clean out all of the "duplicate" zero initializers in the fs-specific code was accepted...
Quoted 5 lines
What I would like to avoid is scenario like:
Maintainers of filesystems with large private inodes: Why would we separate them? We would only waste memory, since the other filesystems stay in ->u and keep it large.
Well, if we get rid of NFS (50 x __u32) and HFS (44 * __u32) (sizes are approximate for 32-bit arches - I was just counting by hand and not strictly checking alignment), then almost all other filesystems are below 25 * __u32 (i.e. half of the previous size).
For large-private-inode filesystems, we are wasting memory in EVERY inode in the slab cache, not just ones in use with the large private inode. If it were the most common filesystem (ext2, maybe reiser, msdos next) then it wouldn't make much difference.
At some point reducing the union size is not efficient to have separate slab allocations from a memory usage standpoint.
The remaining info structs are (approx. for 32-bit arch) (size in __u32):
ext2 27
affs 26
ufs 25
socket 24
shmem 22
coda 20
qnx4 18
minix 16
umsdos 15
hpfs 15
efs 14
sysv 13
reiser 12
udf 12
ntfs 11
ncp 10
msdos 9
adfs 7
smb 6
usbdev 5
proc 4
iso 4
bfs 3
romfs 2
Quoted 2 lines
Maintainers of the rest of filesystems: Since there's no patches that would take large stuff out of ->u, why would we bother?
Maybe the size of the union can depend on CONFIG_*_FS? There should be an absolute minimum size (16 * __u32 or so), but then people who want reiserfs as their primary fs do not need to pay the memory penalty of ext2. For ext2 (the next largest and most common fs), we could make it part of the union if it is compiled in, and on a slab cache if it is a module?
Should uncommon-but-widely-used things like socket and shmem have their own slab cache, or should they just allocate from the generic size-32 slab?
Cheers, Andreas
Signature · 3 lines
Andreas Dilger \ "If a man ate a pound of pasta and a pound of antipasto,
\ would they cancel out, leaving him still hungry?"
http://www-mddsp.enel.ucalgary.ca/People/adilger/ -- DogbertQuoted 4 lines · Andreas Dilger
One thing to watch out for is that the current code zeros the u. struct for us (as you pointed out to me previously), but allocating from the slab cache will not... This could be an interesting source of bugs for some filesystems that assume zero'd
inode_infostructs.
True, but easy to catch.
Quoted 4 lines
Well, if we get rid of NFS (50 x
__u32) and HFS (44 *__u32) (sizes are approximate for 32-bit arches - I was just counting by hand and not strictly checking alignment), then almost all other filesystems are below 25 *__u32(i.e. half of the previous size).
Yeah, but NFS suddenly takes 25+50 words... That's the type of complaints I'm thinking about.
Quoted 5 lines
Maybe the size of the union can depend on CONFIG_*_FS? There should be an absolute minimum size (16 *
__u32or so), but then people who want reiserfs as their primary fs do not need to pay the memory penalty of ext2. For ext2 (the next largest and most common fs), we could make it part of the union if it is compiled in, and on a slab cache if it is a module?
NO. Sorry about shouting, but that's the way to madness. I can understand code depending on SMP vs. UP and similar beasts, but presense of specific filesystems.... <shudder>
Quoted 2 lines
Should uncommon-but-widely-used things like socket and shmem have their own slab cache, or should they just allocate from the generic size-32 slab?
That's pretty interesting - especially for sockets. I wonder whether we would get problems with separate allocation of these - we don't go from inode to socket all that often, but...
Quoted 7 lines · Al
Quoted 4 lines
Well, if we get rid of NFS (50 x
__u32) and HFS (44 *__u32) (sizes are approximate for 32-bit arches - I was just counting by hand and not strictly checking alignment), then almost all other filesystems are below 25 *__u32(i.e. half of the previous size).Yeah, but NFS suddenly takes 25+50 words... That's the type of complaints I'm thinking about.
But then again, you are saving 50-25 words for every non-NFS inode, and I think _most_ systems will have more local inodes than NFS inodes. Even NFS servers will have local inodes, only clients (AFAIK) use nfs_inode_info.
Quoted 9 lines
Quoted 5 lines
Maybe the size of the union can depend on CONFIG_*_FS? There should be an absolute minimum size (16 *
__u32or so), but then people who want reiserfs as their primary fs do not need to pay the memory penalty of ext2. For ext2 (the next largest and most common fs), we could make it part of the union if it is compiled in, and on a slab cache if it is a module?NO. Sorry about shouting, but that's the way to madness. I can understand code depending on SMP vs. UP and similar beasts, but presense of specific filesystems.... <shudder>
But then again, if the size of nfs_inode_info changes, it is the same problem... sizeof(struct inode) may have changed (depends if slab has some padding between inodes or not). If we stick to a minimum size (16 words or maybe even 8), then it will never change anymore, and we do not have overhead for small inode_info structs.
Quoted 6 lines
Quoted 2 lines
Should uncommon-but-widely-used things like socket and shmem have their own slab cache, or should they just allocate from the generic size-32 slab?
That's pretty interesting - especially for sockets. I wonder whether we would get problems with separate allocation of these - we don't go from inode to socket all that often, but...
I never thought of that. I guess the socket code does not know which fs the inode_info was allocated from, so it cannot free it from the slab (even if it had access to these slabs, which it does not). In that case, each fs would have struct socket as the minimum size allocatable, which is unfortunately one of the largest inode_info sizes. It is smaller than ext2, but...
Any ideas? Do we ever get back into fs-specific clear_inode() from a socket? In that case, the socket would just hold a pointer to the fs-specific inode_info inside its own struct socket until the inode is dropped.
Cheers, Andreas
Signature · 3 lines
Andreas Dilger \ "If a man ate a pound of pasta and a pound of antipasto,
\ would they cancel out, leaving him still hungry?"
http://www-mddsp.enel.ucalgary.ca/People/adilger/ -- DogbertQuoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
" " == Alexander Viro <[email protected]> writes:
> On Mon, 23 Apr 2001, Jan Harkes wrote:
>> On Mon, Apr 23, 2001 at 10:45:05PM +0200, Ingo Oeser wrote:
>> > BTW: Is it still less than one page? Then it doesn't make me
>> > nervous. Why? Guess what granularity we allocate at, if we
>> > just store pointers instead of the inode.u. Or do you like
>> > every FS creating his own slab cache?
> Oh, for crying out loud. All it takes is half an hour per > filesystem. Here - completely untested patch that does it for > NFS. Took about that long. Absolutely straightforward, very > easy to verify correctness.
> Some stuff may need tweaking, but not much (e.g. some functions > should take nfs_inode_info instead of inodes, etc.). From the > look of flushd cache it seems that we would be better off with > cyclic lists instead of single-linked ones for the hash, but I > didn't look deep enough.
> So consider the patch below as proof-of-concept. Enjoy:
Hi Al,
I believe your patch introduces a race for the NFS case. The problem
lies in the fact that nfs_find_actor() needs to read several of the fields from nfs_inode_info. By adding an allocation after the inode has been hashed, you are creating a window during which the inode can be found by find_inode(), but during which you aren't even guaranteed that the nfs_inode_info exists let alone that it's been initialized by nfs_fill_inode().
One solution could be to have find_inode sleep on encountering a locked inode. It would have to be something along the lines of
static struct inode * find_inode(struct super_block * sb, unsigned long ino, struct list_head *head, find_inode_t find_actor, void *opaque)
{
struct list_head *tmp;
struct inode * inode;
tmp = head;
for (;;) {
tmp = tmp->next;
inode = NULL;
if (tmp == head)
break;
inode = list_entry(tmp, struct inode, i_hash);
if (inode->i_ino != ino)
continue;
if (inode->i_sb != sb)
continue;
if (find_actor) {
if (inode->i_state & I_LOCK) {
spin_unlock(&inode_lock);
__wait_on_inode(inode);
spin_lock(&inode_lock);
tmp = head;
continue;
}
if (!find_actor(inode, ino, opaque))
continue;
}
break;
}
return inode;
}Cheers,
Trond
Quoted 6 lines · Alexander Viro
Quoted 4 lines · Andreas Dilger
One thing to watch out for is that the current code zeros the u. struct for us (as you pointed out to me previously), but allocating from the slab cache will not... This could be an interesting source of bugs for some filesystems that assume zero'd
inode_infostructs.True, but easy to catch.
Jepp. Just request SLAB_ZERO (still to be implemented) instead of SLAB_POISON or provide an constructor.
A nice set of macros for this would make it quite easy. The ctor is the way to handle it. May be we could even put all the fs specific initalizers into it (e.g. magics, zeroes).
Regards
Ingo Oeser
Signature · 2 lines
10.+11.03.2001 - 3. Chemnitzer LinuxTag <http://www.tu-chemnitz.de/linux/tag>
<<<<<<<<<<<< been there and had much fun >>>>>>>>>>>>Quoted 9 lines · Trond Myklebust
Hi Al,
I believe your patch introduces a race for the NFS case. The problem
lies in the fact thatnfs_find_actor()needs to read several of the fields fromnfs_inode_info. By adding an allocation after the inode has been hashed, you are creating a window during which the inode can be found byfind_inode(), but during which you aren't even guaranteed that thenfs_inode_infoexists let alone that it's been initialized bynfs_fill_inode().
_Ouch_. So what are you going to do if another iget4() comes between the moment when you hash the inode and set these fields? You are filling them only after you drop inode_lock, so AFAICS the current code has the same problem.
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
" " == Alexander Viro <[email protected]> writes:
> _Ouch_. So what are you going to do if another iget4() comes > between the moment when you hash the inode and set these > fields? You are filling them only after you drop inode_lock, so > AFAICS the current code has the same problem.
The entire call to iget4() is protected by the BKL in all relevant instances. As long as we don't sleep between find_inode() and nfs_fill_inode(), we're safe.
In fact the BKL protection is needed also for another reason: we don't actually initialize the inode in the I_LOCK-protected read_inode() but instead rely on the caller of iget4 to do it for us. The reason is that one we would need to pass the struct nfs_fattr to read_inode() and this wasn't possible until the ReiserFS people introduced read_inode2().
Cheers,
Trond
PATCH 1/1 encapsulate shmem access to shmem_inode_info
In reply to Alexander ViroHi,
Quoted 10 lines · Christoph Rohland
Hi Al,
Quoted 4 lines · Alexander Viro
So yes, IMO having such patches available _is_ a good thing. And in 2.5 we definitely want them in the tree. If encapsulation part gets there during 2.4 and separate allocation is available for all of them it will be easier to do without PITA in process.
OK I will do that for tmpfs soon. And I will do the symlink inlining with that patch.
Here comes the patch to encapsulate all accesses to struct shmem_inode_info into a macro. It is now trivial to allocate the private part independently from the inode.
Greetings
Christoph
P.S: The symlink inlining will come in a separate patch
include/linux/shmem_fs.h +10 -7
@@ -18,14 +18,15 @@ } swp_entry_t;struct shmem_inode_info { -spinlock_t lock; -struct semaphore sem; -unsigned long max_index; -swp_entry_t i_direct[SHMEM_NR_DIRECT ];/* for the first blocks */ -swp_entry_t **i_indirect;/* doubly indirect blocks */ -unsigned long swapped; -int locked;/* into memory */ +spinlock_t lock; +struct semaphore sem; +unsigned long max_index; +swp_entry_t i_direct[SHMEM_NR_DIRECT ];/* for the first blocks */ +swp_entry_t **i_indirect;/* doubly indirect blocks */ +unsigned long swapped; +int locked;/* into memory */ struct list_head list; +struct inode *inode; };struct shmem_sb_info {
@@ -35,5 +36,7 @@unsigned long free_inodes;/* How many are left for allocation */ spinlock_t stat_lock; }; + +#define SHMEM_I (inode) (&inode->u.shmem_i)#endif
ipc/shm.c +5 -3
@@ -348,6 +348,7 @@static void shm_get_stat (unsigned long *rss,unsigned long *swp) { +struct shmem_inode_info *info;int i; *rss =0 ;
@@ -361,10 +362,11 @@if (shp ==NULL )continue ; inode = shp->shm_file->f_dentry->d_inode; -spin_lock (&inode->u.shmem_i.lock); + info =SHMEM_I (inode); +spin_lock (&info->lock); *rss += inode->i_mapping->nrpages; - *swp += inode->u.shmem_i.swapped; -spin_unlock (&inode->u.shmem_i.lock); + *swp += info->swapped; +spin_unlock (&info->lock); } }
mm/shmem.c +19 -19
@@ -73,7 +73,7 @@unsigned long freed; freed = (inode->i_blocks/BLOCKS_PER_PAGE ) - - (inode->i_mapping->nrpages + inode->u.shmem_i.swapped); + (inode->i_mapping->nrpages +SHMEM_I (inode)->swapped);if (freed){struct shmem_sb_info * info = &inode->i_sb->u.shmem_sb; inode->i_blocks -= freed*BLOCKS_PER_PAGE ;
@@ -159,7 +159,7 @@unsigned long index, start;unsigned long freed =0 ;swp_entry_t **base, **ptr, **last; -struct shmem_inode_info * info = &inode->u.shmem_i; +struct shmem_inode_info * info =SHMEM_I (inode);down (&info->sem); inode->i_ctime = inode->i_mtime =CURRENT_TIME ;
@@ -206,7 +206,7 @@struct shmem_sb_info *info = &inode->i_sb->u.shmem_sb;spin_lock (&shmem_ilock); -list_del (&inode->u.shmem_i.list); +list_del (&SHMEM_I (inode)->list);spin_unlock (&shmem_ilock); inode->i_size =0 ;shmem_truncate (inode);
@@ -239,7 +239,7 @@ goto out; inode = page->mapping->host; - info = &inode->u.shmem_i; + info =SHMEM_I (inode); swap =__get_swap_page (2 ); error = -ENOMEM ;if (!swap.val)
@@ -407,7 +407,7 @@page_cache_release (*ptr); } - info = &inode->u.shmem_i; + info =SHMEM_I (inode);down (&info->sem);/* retest we may have slept */
@@ -415,7 +415,7 @@if (inode->i_size < (loff_t ) idx *PAGE_CACHE_SIZE ) goto failed; - *ptr =shmem_getpage_locked (&inode->u.shmem_i, inode, idx); + *ptr =shmem_getpage_locked (info, inode, idx);if (IS_ERR (*ptr)) goto failed;
@@ -462,7 +462,7 @@void shmem_lock (struct file * file,int lock) {struct inode * inode = file->f_dentry->d_inode; -struct shmem_inode_info * info = &inode->u.shmem_i; +struct shmem_inode_info * info =SHMEM_I (inode);struct page * page;unsigned long idx, size;
@@ -521,7 +521,8 @@ inode->i_rdev =to_kdev_t (dev); inode->i_mapping->a_ops = &shmem_aops; inode->i_atime = inode->i_mtime = inode->i_ctime =CURRENT_TIME ; - info = &inode->u.shmem_i; + info =SHMEM_I (inode); + info->inode = inode;spin_lock_init (&info->lock);sema_init (&info->sem,1 );switch (mode &S_IFMT ) {
@@ -542,7 +543,7 @@break ; }spin_lock (&shmem_ilock); -list_add (&inode->u.shmem_i.list, &shmem_inodes); +list_add (&SHMEM_I (inode)->list, &shmem_inodes);spin_unlock (&shmem_ilock); }return inode;
@@ -629,7 +630,7 @@__get_user (dummy, buf+bytes-1 ); } - info = &inode->u.shmem_i; + info =SHMEM_I (inode);down (&info->sem); page =shmem_getpage_locked (info, inode, index);up (&info->sem);
@@ -658,8 +659,8 @@ buf += bytes;if (pos > inode->i_size) inode->i_size = pos; -if (inode->u.shmem_i.max_index <= index) - inode->u.shmem_i.max_index = index+1 ; +if (info->max_index <= index) + info->max_index = index+1 ; } unlock:
@@ -940,7 +941,7 @@ inode = dentry->d_inode;down (&inode->i_sem); - page =shmem_getpage_locked (&inode->u.shmem_i, inode,0 ); + page =shmem_getpage_locked (SHMEM_I (inode), inode,0 );if (IS_ERR (page)) goto fail; kaddr =kmap (page);
@@ -1220,12 +1221,11 @@return -1 ; } -static int shmem_unuse_inode (struct inode *inode,swp_entry_t entry,struct page *page) +static int shmem_unuse_inode (struct shmem_inode_info *info,swp_entry_t entry,struct page *page) {swp_entry_t **base, **ptr;unsigned long idx;int offset; -struct shmem_inode_info *info = &inode->u.shmem_i; idx =0 ;spin_lock (&info->lock);
@@ -1246,7 +1246,7 @@spin_unlock (&info->lock);return 0 ; found: -add_to_page_cache (page, inode->i_mapping, offset + idx); +add_to_page_cache (page, info->inode->i_mapping, offset + idx);set_page_dirty (page);SetPageUptodate (page);UnlockPage (page);
@@ -1261,13 +1261,13 @@void shmem_unuse (swp_entry_t entry,struct page *page) {struct list_head *p; -struct inode * inode; +struct shmem_inode_info * info;spin_lock (&shmem_ilock);list_for_each (p, &shmem_inodes) { - inode = list_entry(p, struct inode, u.shmem_i.list); + info = list_entry(p, struct shmem_inode_info, list); -if (shmem_unuse_inode (inode, entry, page)) +if (shmem_unuse_inode (info, entry, page))break ; }spin_unlock (&shmem_ilock);
PATCH 1/1 inline symlinks for tmpfs
In reply to Alexander ViroHi David,
Quoted 2 lines · David L. Parsley
Quoted 2 lines
OK I will do that for tmpfs soon. And I will do the symlink inlining with that patch.
OK, here comes the patch for the symlink inlining. It is on top of my previous patch to encapsulate access to the private inode info.
Greetings
Christoph
mm/shmem.c +61 -33
@@ -41,7 +41,6 @@static struct inode_operations shmem_inode_operations;static struct file_operations shmem_dir_operations;static struct inode_operations shmem_dir_inode_operations; -static struct inode_operations shmem_symlink_inode_operations;static struct vm_operations_struct shmem_vm_ops;LIST_HEAD (shmem_inodes);
@@ -205,11 +204,13 @@ {struct shmem_sb_info *info = &inode->i_sb->u.shmem_sb; -spin_lock (&shmem_ilock); -list_del (&SHMEM_I (inode)->list); -spin_unlock (&shmem_ilock); inode->i_size =0 ; -shmem_truncate (inode); +if (inode->i_op->truncate == shmem_truncate){ +spin_lock (&shmem_ilock); +list_del (&SHMEM_I (inode)->list); +spin_unlock (&shmem_ilock); +shmem_truncate (inode); + }spin_lock (&info->stat_lock); info->free_inodes++;spin_unlock (&info->stat_lock);
@@ -532,6 +533,9 @@case S_IFREG : inode->i_op = &shmem_inode_operations; inode->i_fop = &shmem_file_operations; +spin_lock (&shmem_ilock); +list_add (&SHMEM_I (inode)->list, &shmem_inodes); +spin_unlock (&shmem_ilock);break ;case S_IFDIR : inode->i_nlink++;
@@ -539,17 +543,17 @@ inode->i_fop = &shmem_dir_operations;break ;case S_IFLNK : - inode->i_op = &shmem_symlink_inode_operations;break ; } -spin_lock (&shmem_ilock); -list_add (&SHMEM_I (inode)->list, &shmem_inodes); -spin_unlock (&shmem_ilock); }return inode; }#ifdef CONFIG_TMPFS + +static struct inode_operations shmem_symlink_inode_operations; +static struct inode_operations shmem_symlink_inline_operations; +static ssize_t shmem_file_write (struct file *file,const char *buf,size_t count,loff_t *ppos) {
@@ -930,33 +934,54 @@struct inode *inode;struct page *page;char *kaddr; +struct shmem_inode_info * info; error =shmem_mknod (dir, dentry,S_IFLNK |S_IRWXUGO ,0 );if (error)return error; - len =strlen (symname); + len =strlen (symname) +1 ;if (len >PAGE_SIZE )return -ENAMETOOLONG ; - + inode = dentry->d_inode; -down (&inode->i_sem); - page =shmem_getpage_locked (SHMEM_I (inode), inode,0 ); -if (IS_ERR (page)) - goto fail; - kaddr =kmap (page); -memcpy (kaddr, symname, len); -kunmap (page); + info =SHMEM_I (inode); inode->i_size = len; -SetPageDirty (page); -UnlockPage (page); -page_cache_release (page); -up (&inode->i_sem); +if (len <=sizeof (struct shmem_inode_info )) { +/* do it inline */ +memcpy (info, symname, len); + inode->i_op = &shmem_symlink_inline_operations; + }else { +spin_lock (&shmem_ilock); +list_add (&info->list, &shmem_inodes); +spin_unlock (&shmem_ilock); +down (&inode->i_sem); + page =shmem_getpage_locked (info, inode,0 ); +if (IS_ERR (page)) { +up (&inode->i_sem); +return PTR_ERR (page); + } + kaddr =kmap (page); +memcpy (kaddr, symname, len); +kunmap (page); +SetPageDirty (page); +UnlockPage (page); +page_cache_release (page); +up (&inode->i_sem); + inode->i_op = &shmem_symlink_inode_operations; + } dir->i_ctime = dir->i_mtime =CURRENT_TIME ;return 0 ; -fail: -up (&inode->i_sem); -return PTR_ERR (page); +} + +static int shmem_readlink_inline (struct dentry *dentry,char *buffer,int buflen) +{ +return vfs_readlink (dentry,buffer,buflen, (const char *)SHMEM_I (dentry->d_inode)); +} + +static int shmem_follow_link_inline (struct dentry *dentry,struct nameidata *nd) +{ +return vfs_follow_link (nd, (const char *)SHMEM_I (dentry->d_inode)); }static int shmem_readlink (struct dentry *dentry,char *buffer,int buflen)
@@ -986,6 +1011,17 @@return res; } +static struct inode_operations shmem_symlink_inline_operations = { + readlink: shmem_readlink_inline, + follow_link: shmem_follow_link_inline, +}; + +static struct inode_operations shmem_symlink_inode_operations = { + truncate: shmem_truncate, + readlink: shmem_readlink, + follow_link: shmem_follow_link, +}; +static int shmem_parse_options (char *options,int *mode,unsigned long * blocks,unsigned long *inodes) {char *this_char, *value;
@@ -1118,14 +1154,6 @@static struct inode_operations shmem_inode_operations = { truncate: shmem_truncate, -}; - -static struct inode_operations shmem_symlink_inode_operations = { - truncate: shmem_truncate, -#ifdef CONFIG_TMPFS - readlink: shmem_readlink, - follow_link: shmem_follow_link, -#endif };static struct file_operations shmem_dir_operations = {
PATCH RESEND 1/1 Collection of tmpfs patches
In reply to Alexander ViroHi,
There is some confusion about my latest tmpfs fixes. There were three patches which are cummulative against 2.4.4:
1) deadlock fix for write out of mmap regions. (AFAIK this is integrated in the -ac kernels)
2) encapsulate access to shmem_inode_info
3) Do inline symlinks
I attach all these patches to this mail in the case that somebody missed one.
Greetings
Christoph
include/linux/shmem_fs.h +1 -0
@@ -19,6 +19,7 @@struct shmem_inode_info {spinlock_t lock; +struct semaphore sem;unsigned long max_index;swp_entry_t i_direct[SHMEM_NR_DIRECT ];/* for the first blocks */ swp_entry_t **i_indirect;/* doubly indirect blocks */
mm/shmem.c +29 -21
129 lines not on this page · raw message
include/linux/shmem_fs.h +10 -7
@@ -18,14 +18,15 @@ } swp_entry_t;struct shmem_inode_info { -spinlock_t lock; -struct semaphore sem; -unsigned long max_index; -swp_entry_t i_direct[SHMEM_NR_DIRECT ];/* for the first blocks */ -swp_entry_t **i_indirect;/* doubly indirect blocks */ -unsigned long swapped; -int locked;/* into memory */ +spinlock_t lock; +struct semaphore sem; +unsigned long max_index; +swp_entry_t i_direct[SHMEM_NR_DIRECT ];/* for the first blocks */ +swp_entry_t **i_indirect;/* doubly indirect blocks */ +unsigned long swapped; +int locked;/* into memory */ struct list_head list; +struct inode *inode; };struct shmem_sb_info {
@@ -35,5 +36,7 @@unsigned long free_inodes;/* How many are left for allocation */ spinlock_t stat_lock; }; + +#define SHMEM_I (inode) (&inode->u.shmem_i)#endif
ipc/shm.c +5 -3
@@ -348,6 +348,7 @@static void shm_get_stat (unsigned long *rss,unsigned long *swp) { +struct shmem_inode_info *info;int i; *rss =0 ;
@@ -361,10 +362,11 @@if (shp ==NULL )continue ; inode = shp->shm_file->f_dentry->d_inode; -spin_lock (&inode->u.shmem_i.lock); + info =SHMEM_I (inode); +spin_lock (&info->lock); *rss += inode->i_mapping->nrpages; - *swp += inode->u.shmem_i.swapped; -spin_unlock (&inode->u.shmem_i.lock); + *swp += info->swapped; +spin_unlock (&info->lock); } }
mm/shmem.c +19 -19
@@ -73,7 +73,7 @@unsigned long freed; freed = (inode->i_blocks/BLOCKS_PER_PAGE ) - - (inode->i_mapping->nrpages + inode->u.shmem_i.swapped); + (inode->i_mapping->nrpages +SHMEM_I (inode)->swapped);if (freed){struct shmem_sb_info * info = &inode->i_sb->u.shmem_sb; inode->i_blocks -= freed*BLOCKS_PER_PAGE ;
@@ -159,7 +159,7 @@unsigned long index, start;unsigned long freed =0 ;swp_entry_t **base, **ptr, **last; -struct shmem_inode_info * info = &inode->u.shmem_i; +struct shmem_inode_info * info =SHMEM_I (inode);down (&info->sem); inode->i_ctime = inode->i_mtime =CURRENT_TIME ;
@@ -206,7 +206,7 @@struct shmem_sb_info *info = &inode->i_sb->u.shmem_sb;spin_lock (&shmem_ilock); -list_del (&inode->u.shmem_i.list); +list_del (&SHMEM_I (inode)->list);spin_unlock (&shmem_ilock); inode->i_size =0 ;shmem_truncate (inode);
@@ -239,7 +239,7 @@ goto out; inode = page->mapping->host; - info = &inode->u.shmem_i; + info =SHMEM_I (inode); swap =__get_swap_page (2 ); error = -ENOMEM ;if (!swap.val)
@@ -407,7 +407,7 @@page_cache_release (*ptr); } - info = &inode->u.shmem_i; + info =SHMEM_I (inode);down (&info->sem);/* retest we may have slept */
@@ -415,7 +415,7 @@if (inode->i_size < (loff_t ) idx *PAGE_CACHE_SIZE ) goto failed; - *ptr =shmem_getpage_locked (&inode->u.shmem_i, inode, idx); + *ptr =shmem_getpage_locked (info, inode, idx);if (IS_ERR (*ptr)) goto failed;
@@ -462,7 +462,7 @@void shmem_lock (struct file * file,int lock) {struct inode * inode = file->f_dentry->d_inode; -struct shmem_inode_info * info = &inode->u.shmem_i; +struct shmem_inode_info * info =SHMEM_I (inode);struct page * page;unsigned long idx, size;
@@ -521,7 +521,8 @@ inode->i_rdev =to_kdev_t (dev); inode->i_mapping->a_ops = &shmem_aops; inode->i_atime = inode->i_mtime = inode->i_ctime =CURRENT_TIME ; - info = &inode->u.shmem_i; + info =SHMEM_I (inode); + info->inode = inode;spin_lock_init (&info->lock);sema_init (&info->sem,1 );switch (mode &S_IFMT ) {
@@ -542,7 +543,7 @@break ; }spin_lock (&shmem_ilock); -list_add (&inode->u.shmem_i.list, &shmem_inodes); +list_add (&SHMEM_I (inode)->list, &shmem_inodes);spin_unlock (&shmem_ilock); }return inode;
@@ -629,7 +630,7 @@__get_user (dummy, buf+bytes-1 ); } - info = &inode->u.shmem_i; + info =SHMEM_I (inode);down (&info->sem); page =shmem_getpage_locked (info, inode, index);up (&info->sem);
@@ -658,8 +659,8 @@ buf += bytes;if (pos > inode->i_size) inode->i_size = pos; -if (inode->u.shmem_i.max_index <= index) - inode->u.shmem_i.max_index = index+1 ; +if (info->max_index <= index) + info->max_index = index+1 ; } unlock:
@@ -940,7 +941,7 @@ inode = dentry->d_inode;down (&inode->i_sem); - page =shmem_getpage_locked (&inode->u.shmem_i, inode,0 ); + page =shmem_getpage_locked (SHMEM_I (inode), inode,0 );if (IS_ERR (page)) goto fail; kaddr =kmap (page);
@@ -1220,12 +1221,11 @@return -1 ; } -static int shmem_unuse_inode (struct inode *inode,swp_entry_t entry,struct page *page) +static int shmem_unuse_inode (struct shmem_inode_info *info,swp_entry_t entry,struct page *page) {swp_entry_t **base, **ptr;unsigned long idx;int offset; -struct shmem_inode_info *info = &inode->u.shmem_i; idx =0 ;spin_lock (&info->lock);
@@ -1246,7 +1246,7 @@spin_unlock (&info->lock);return 0 ; found: -add_to_page_cache (page, inode->i_mapping, offset + idx); +add_to_page_cache (page, info->inode->i_mapping, offset + idx);set_page_dirty (page);SetPageUptodate (page);UnlockPage (page);
@@ -1261,13 +1261,13 @@void shmem_unuse (swp_entry_t entry,struct page *page) {struct list_head *p; -struct inode * inode; +struct shmem_inode_info * info;spin_lock (&shmem_ilock);list_for_each (p, &shmem_inodes) { - inode = list_entry(p, struct inode, u.shmem_i.list); + info = list_entry(p, struct shmem_inode_info, list); -if (shmem_unuse_inode (inode, entry, page)) +if (shmem_unuse_inode (info, entry, page))break ; }spin_unlock (&shmem_ilock);
mm/shmem.c +61 -33
155 lines not on this page · raw message