Re: 2.1.118 Tons of oopes

George (greerga@nidhogg.ham.muohio.edu)
Fri, 28 Aug 1998 11:56:27 -0400 (EDT)


On Fri, 28 Aug 1998, Richard Gooch wrote:

>Look, I am entitled to think differently than you. I've been
>listening. It seems to me people haven't listened to my points.

I'll refrain from anything here.

>What I don't understand is *why* this flush() method is termed
>mandatory (hence the intentional source breakage), and yet in *every*
>driver in the 2.1.118 patch it is set to NULL.

If you go poking in include/linux/fs.h, you notice this structure:

struct file_operations {
loff_t (*llseek) (struct file *, loff_t, int);
ssize_t (*read) (struct file *, char *, size_t, loff_t *);
ssize_t (*write) (struct file *, const char *, size_t, loff_t *);
int (*readdir) (struct file *, void *, filldir_t);
unsigned int (*poll) (struct file *, struct poll_table_struct *);
int (*ioctl) (struct inode *, struct file *, unsigned int, unsigned long);
int (*mmap) (struct file *, struct vm_area_struct *);
int (*open) (struct inode *, struct file *);
int (*flush) (struct file *);
int (*release) (struct inode *, struct file *);
int (*fsync) (struct file *, struct dentry *);
int (*fasync) (int, struct file *, int);
int (*check_media_change) (kdev_t dev);
int (*revalidate) (kdev_t dev);
int (*lock) (struct file *, int, struct file_lock *);
};

Now, in the interests of keeping everything clean, modular, and uniform,
the flush() method was added to the file operations structure. Since
no one but NFS has apparantly needed this operation until now, they are all
set to NULL to say "I don't need anything special." NFS does require it:

/*
* Flush all dirty pages, and check for write errors.
*
* We should probably do this better - this does get called at every
* close, so we should probably just flush the changes that "this"
* file has done and report on only those.
*
* Right now we use the "flush everything" approach. Overkill, but
* works.
*/
static int nfs_file_flush(struct file *file)
{
return nfs_file_close(file->f_dentry->d_inode, file);
}

So, with the magic flush() method, we can tell _before we close the file_
if there were problems writing it. Previously you'll remember the "I lose
data when there is an error on the closing write" NFS problem.

>So what is the difference between breaking all source and then inserting
>NULLs, or appending the new method and taking advantage of automatic
>structure initialisation. The only difference I've seen is that one
>approach breaks things and requires lots of effort.

See above, it fixes a very nasty bug in a way that others can also take
advantage of. Now, if any other network filesystem, such as Coda, needs a
special flush() method, they just add it to the structure. Anyone else who
doesn't need it can just stuff a NULL in there and be done with it. Or
perhaps Reiserfs would like one which isn't in the kernel. It's there for
the "hey, look at me" factor. Then after you notice it, you put a NULL in
if you don't care.

>Again, if there is some other hidden assumption going on here, I'd like to
>know. But *nobody* has explained how the breakage has helped anything.

See above.

>It would have been a different matter is a significant proportion of
>drivers were modified by the patch to now have real flush() methods. But
>that isn't the case: they're all set to NULL. The only conclusion I can
>make from this is that flush() is optional for almost every driver.

They don't need one, but NFS does. Otherwise there will be all sorts of
special file operation hacks all over the kernel.

>No, David, it's not a case of me not listening. It's a case of people
>not explaining things clearly. If I'm wrong in flush() being optional,
>then there is some hidden assumption.

It's optional.

>Maybe the inner circle *does* know some deep dark secret, but *I*
>don't know it and it hasn't been made public. In light of this, my
>questions are entirely reasonable. Instead of flaming me for pursuing
>this, why not come forth with some clearer explanations?

Well, I can count the number of kernel things I've done on one hand and I
just kind of read the source. :)

-George

-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.rutgers.edu
Please read the FAQ at http://www.altern.org/andrebalsa/doc/lkml-faq.html