On Mon, Jul 24, 2000 at 02:24:42PM -0400, Jan Harkes wrote:
> On Mon, Jul 24, 2000 at 12:04:39PM -0400, Benjamin C.R. LaHaise wrote:
> > On Mon, 24 Jul 2000, Alexander Viro wrote:
> >
> > > Not needed. That race is taken care of - readpage() never does access past
> > > i_size and has the page locked, while vmtruncate() starts with setting
> > > i_size (on shrinking truncate(), that is), then goes through all pages in
> > > affected area and grabs the lock on all of them (one by one, not
> > > simultaneously, indeed) and only then calls ->truncate().
> >
> > That assumes that i_size is atomically updated, which currently is not the
> > case on 32 bit platforms.
>
> I found that i_size is in most cases reasonably protected by i_sem.
> Except for Coda where inode->i_size != inode->i_mapping->host->i_size,
> because we `borrow' the mapping of the underlying container file.
>
> At the moment I believe that the introduction of i_mapping->host might
> have been a mistake. The readpage/writepage operations use
> i_mapping->host->i_size, but sys_stat and generic_write_file (for
> a file with O_APPEND) are using inode->i_size. So for Coda we are
> copying the i_size back to the Coda inode in a wrapper around
> generic_file_write.
>
> AFAIK Coda is the only FS that has an address space mapping that
> associated with another inode, and we need to lock the i_sem of that
> inode to avoid problems. Maybe the Coda file write should become
> something like:
>
> coda_file_write()
> {
> down(&container_inode->i_sem);
> n = generic_file_write(file, ...);
> container_inode->i_size = file->f_dentry->d_inode->i_size;
> up(&container_inode->i_sem);
> return n;
> }
>
> That way all writes are protected by the semaphore on the container
> file's inode. And the use of i_mapping->host, along with the nasty cast
> to (struct inode *) could be removed.
This might work for Coda, but I think in general there are a couple of
problems with always using the struct file to get at the inode
structure. IIRC either writepage or prepare_write is called with
file==NULL in some circumstances (when creating a symlink I think). I
discovered this when I got oopsen in my patch which adds
space/inode/dentry usage limits to ramfs.
There's another problem too: for ramfs limits, I obviously also need
to keep track of when a page is removed from an inode. The only sane
way I've seen to do this is to add a hook to address_space_operations
called when the page is removed (currently I'm calling it from
remove_page_from_inode_queue() but that may not be the best place). At
this point, only the struct page is available, so from that I need to
find the inode (well, actually inode->i_sb where I store the
limits). Easy with mapping->host, otherwise impossible (AFAICT, it's
entirely possible I'm missing something blindingly obvious).
--
David Gibson, Technical Support Engineer, Linuxcare, Inc.
+61 2 6262 8990
[EMAIL PROTECTED], http://www.linuxcare.com/
Linuxcare. Support for the revolution.
