Messages in this thread | | | Date | Mon, 7 Jan 2008 17:39:48 +0100 | From | Jan Kara <> | Subject | Re: [PATCH] Handle i_size > s_maxbytes correctly |
| |
On Sat 22-12-07 00:12:06, Andrew Morton wrote: Sorry for a late reply but I was on vacation.
> On Thu, 20 Dec 2007 18:51:04 +0100 Jan Kara <jack@suse.cz> wrote: > > > Although we don't allow writes over s_maxbytes, it can happen that a file's > > size is larger than s_maxbytes. For example we can write the file from a > > computer with a different architecture (which has larger s_maxbytes), boot > > a kernel with a different set of config options (CONFIG_LBD...), or if two > > nodes in a [Ocfs2, and likely Gfs2] cluster have mounted the same file > > system and have different s_maxbytes. Thus we have to make sure we don't > > crash / corrupt data when seeing such file (page offset of the last page > > needn't fit into pgoff_t). Firstly, we make read() and mmap() return error > > when user tries to access the file above s_maxbytes, secondly we introduce > > a function i_size_read_trunc() which returns min(i_size, s_maxbytes) and > > use it when determining maximal page offset we are interested in. > > > > ... > > > > --- a/fs/buffer.c > > +++ b/fs/buffer.c > > @@ -1623,7 +1623,7 @@ static int __block_write_full_page(struct inode *inode, struct page *page, > > > > BUG_ON(!PageLocked(page)); > > > > - last_block = (i_size_read(inode) - 1) >> inode->i_blkbits; > > + last_block = (i_size_read_trunc(inode) - 1) >> inode->i_blkbits; > > > > ... > > > > +/* Truncate i_size at s_maxbytes so that pagecache doesn't have problems */ > > +static inline loff_t i_size_read_trunc(const struct inode *inode) > > +{ > > + loff_t i_size = i_size_read(inode); > > + > > + if (unlikely(inode->i_sb->s_maxbytes < i_size)) > > + return inode->i_sb->s_maxbytes; > > + return i_size; > > +} > > + > > This patch takes the total text size of the affected nine files from 74167 > bytes up to 75066 on i386. This is core, core kernel. Ouch. > > It's also pretty fragile. We now have i_size_read()s and > i_size_read_trunc()s sprinkled all over the place with no obvious rules to > determine when we should use one versus the other. Looking at the patch from the distance of two weeks I agree this is a flaw...
> uninlining i_size_read_trunc() is obviously the first thing to look at but > the cost is still appreciable and boy the problem which is being fixed here > is rare and obscure. > > Can we look at alternatives please? What about just failing the open > attempt? As Mark wrote, just failing the open does not solve the problem for clustered filesystems. I'll try to come up with something that would be a less fragile solution (and won't increase the text size that much).
Thanks for your comments
Honza -- Jan Kara <jack@suse.cz> SUSE Labs, CR
| |