why does fsync() on a tmpfs directory give EINVAL?

why does fsync() on a tmpfs directory give EINVAL?

[Chris Friesen]

The man page for fsync() suggests that it is necessary to call it on the 
directory fd.

However, in the case of tmpfs, fsync() on the file completes, but on the 
directory it returns -1 with errno==EINVAL.

Is there any particular reason for this?  Would a patch that makes it 
just return successfully without doing anything be accepted?

Chris

Re: why does fsync() on a tmpfs directory give EINVAL?

[Andrew Morton]

Chris Friesen <cfriesen@nortel.com> wrote:
>
> 
> The man page for fsync() suggests that it is necessary to call it on the 
> directory fd.

yup.

> However, in the case of tmpfs, fsync() on the file completes, but on the 
> directory it returns -1 with errno==EINVAL.

bad.

> Is there any particular reason for this?

nope.

>  Would a patch that makes it 
> just return successfully without doing anything be accepted?

yup.

Re: why does fsync() on a tmpfs directory give EINVAL?

[Chris Friesen]

Andrew Morton wrote:
> Chris Friesen <cfriesen@nortel.com> wrote:

>> Would a patch that makes it 
>>just return successfully without doing anything be accepted?
> 
> 
> yup.

Currently tmpfs reuses the simple_dir_operations from libfs.c.

Would it make sense to add the empty fsync() function there, and have 
all other users pick it up as well?  Is this likely to break stuff?

Currently the following code uses simple_dir_operations:

./kernel/cpuset.c
./security/selinux/selinuxfs.c
./ipc/mqueue.c
./include/linux/fs.h
./fs/relayfs/inode.c
./fs/debugfs/inode.c
./fs/ocfs2/dlm/dlmfs.c
./fs/autofs/inode.c
./fs/hugetlbfs/inode.c
./fs/libfs.c
./fs/ramfs/inode.c
./fs/devpts/inode.c
./net/sunrpc/rpc_pipe.c
./mm/shmem.c
./drivers/misc/ibmasm/ibmasmfs.c
./drivers/oprofile/oprofilefs.c
./drivers/isdn/capi/capifs.c
./drivers/usb/core/inode.c
./drivers/usb/gadget/inode.c

Chris

Re: why does fsync() on a tmpfs directory give EINVAL?

[Andrew Morton]

Chris Friesen <cfriesen@nortel.com> wrote:
>
> Andrew Morton wrote:
> > Chris Friesen <cfriesen@nortel.com> wrote:
> 
> >> Would a patch that makes it 
> >>just return successfully without doing anything be accepted?
> > 
> > 
> > yup.
> 
> Currently tmpfs reuses the simple_dir_operations from libfs.c.
> 
> Would it make sense to add the empty fsync() function there, and have 
> all other users pick it up as well?  Is this likely to break stuff?

Isn't simple_sync_file() suitable?

Re: why does fsync() on a tmpfs directory give EINVAL?

[Chris Friesen]

Andrew Morton wrote:
> Chris Friesen <cfriesen@nortel.com> wrote:

>>Currently tmpfs reuses the simple_dir_operations from libfs.c.
>>
>>Would it make sense to add the empty fsync() function there, and have 
>>all other users pick it up as well?  Is this likely to break stuff?
>  
> Isn't simple_sync_file() suitable?

I think it would be fine.  The issue is that currently for directories 
tmpfs doesn't have it's own set of operations--it reuses the 
simple_dir_operations set of file ops from libfs.

We could make a tmpfs-specific set of operations that is identical to 
simple_dir_operations but with the addition of setting the fsync 
function to simple_sync_file().

Alternately, if it makes sense for all the users of 
simple_dir_operations we could modify it directly and all of the other 
users of simple_dir_operations would get the change for free.  I don't 
know enough about the other filesystems to know if this makes sense or not.

Chris

Re: why does fsync() on a tmpfs directory give EINVAL?

[Andrew Morton]

Chris Friesen <cfriesen@nortel.com> wrote:
>
> Andrew Morton wrote:
> > Chris Friesen <cfriesen@nortel.com> wrote:
> 
> >>Currently tmpfs reuses the simple_dir_operations from libfs.c.
> >>
> >>Would it make sense to add the empty fsync() function there, and have 
> >>all other users pick it up as well?  Is this likely to break stuff?
> >  
> > Isn't simple_sync_file() suitable?
> 
> I think it would be fine.  The issue is that currently for directories 
> tmpfs doesn't have it's own set of operations--it reuses the 
> simple_dir_operations set of file ops from libfs.
> 
> We could make a tmpfs-specific set of operations that is identical to 
> simple_dir_operations but with the addition of setting the fsync 
> function to simple_sync_file().
> 
> Alternately, if it makes sense for all the users of 
> simple_dir_operations we could modify it directly and all of the other 
> users of simple_dir_operations would get the change for free.  I don't 
> know enough about the other filesystems to know if this makes sense or not.
> 

hm, what a lot of filesystems.

bix:/usr/src/linux-2.6.12-rc6> grep -rl simple_dir_operations .            
./drivers/usb/gadget/inode.c
./drivers/usb/core/inode.c
./drivers/isdn/capi/capifs.c
./drivers/oprofile/oprofilefs.c
./drivers/misc/ibmasm/ibmasmfs.c
./fs/libfs.c
./fs/debugfs/inode.c
./fs/autofs/inode.c
./fs/devpts/inode.c
./fs/hugetlbfs/inode.c
./fs/ramfs/inode.c
./include/linux/fs.h
./net/sunrpc/rpc_pipe.c
./kernel/cpuset.c
./security/selinux/selinuxfs.c
./ipc/mqueue.c
./mm/shmem.c

I can't think of any reason why any of these would want fsync(dir_fd) to
return -EINVAL.

Re: why does fsync() on a tmpfs directory give EINVAL?

[Chris Wedgwood]

On Thu, Jun 16, 2005 at 06:57:54PM -0700, Andrew Morton wrote:

> hm, what a lot of filesystems.
>
> bix:/usr/src/linux-2.6.12-rc6> grep -rl simple_dir_operations .
> ./drivers/usb/gadget/inode.c
> ./drivers/usb/core/inode.c
> ./drivers/isdn/capi/capifs.c
> ./drivers/oprofile/oprofilefs.c
> ./drivers/misc/ibmasm/ibmasmfs.c
> ./fs/libfs.c
> ./fs/debugfs/inode.c
> ./fs/autofs/inode.c
> ./fs/devpts/inode.c
> ./fs/hugetlbfs/inode.c
> ./fs/ramfs/inode.c
> ./include/linux/fs.h
> ./net/sunrpc/rpc_pipe.c
> ./kernel/cpuset.c
> ./security/selinux/selinuxfs.c
> ./ipc/mqueue.c
> ./mm/shmem.c
>
> I can't think of any reason why any of these would want fsync(dir_fd) to
> return -EINVAL.

Logically I think we can only expect 'real' filesystems with block
devices or similiar behind them to do something with fsync and
everyone else to be more or less undefined.

Undefined creates problems I guess so I guess simple_dir_operations
could return 0 (it sorta does make sense, if you have no backing store
you are always in-sync?).

Re: why does fsync() on a tmpfs directory give EINVAL?

[Hugh Dickins]

On Thu, 16 Jun 2005, Andrew Morton wrote:
> Chris Friesen <cfriesen@nortel.com> wrote:
> > Andrew Morton wrote:
> > > Chris Friesen <cfriesen@nortel.com> wrote:
> > >>Currently tmpfs reuses the simple_dir_operations from libfs.c.
> > >>
> > >>Would it make sense to add the empty fsync() function there, and have 
> > >>all other users pick it up as well?  Is this likely to break stuff?
> > >  
> > > Isn't simple_sync_file() suitable?

Yes.

> > Alternately, if it makes sense for all the users of 
> > simple_dir_operations we could modify it directly and all of the other 
> > users of simple_dir_operations would get the change for free.  I don't 
> > know enough about the other filesystems to know if this makes sense or not.

That makes the best sense, yes.

> hm, what a lot of filesystems.
> ..... 
> I can't think of any reason why any of these would want fsync(dir_fd) to
> return -EINVAL.

No need to check the list: any filesystem using simple_dir_operations
is using dcache_readdir, which implies there's no storage to be synced.
And we all agree that success is a more helpful retval than -EINVAL
when there's nothing for fsync to do.  Here's a patch if you haven't
done it already....

tmpfs, and all other users of simple_dir_operations, should return 0
to say directory fsync was successful, instead of the worrying -EINVAL.

Signed-off-by: Hugh Dickins <hugh@veritas.com>

--- 2.6.12-rc6-git8/fs/libfs.c	2005-03-02 07:38:44.000000000 +0000
+++ linux/fs/libfs.c	2005-06-17 14:16:29.000000000 +0100
@@ -183,6 +183,7 @@ struct file_operations simple_dir_operat
 	.llseek		= dcache_dir_lseek,
 	.read		= generic_read_dir,
 	.readdir	= dcache_readdir,
+	.fsync		= simple_sync_file,
 };
 
 struct inode_operations simple_dir_inode_operations = {

Re: why does fsync() on a tmpfs directory give EINVAL?

[Chris Friesen]

Hugh Dickins wrote:

> No need to check the list: any filesystem using simple_dir_operations
> is using dcache_readdir, which implies there's no storage to be synced.
> And we all agree that success is a more helpful retval than -EINVAL
> when there's nothing for fsync to do.  Here's a patch if you haven't
> done it already....

Thanks for the input.  I wasn't positive the change would be benign, so 
  I was waiting for the discussion to resolve itself.

Chris