Jan Kara [off-list ref] writes:

On Fri 10-10-14 11:48:30, Dmitry Monakhov wrote:

quoted

Andreas Dilger [off-list ref] writes:

quoted

On Oct 9, 2014, at 5:14 AM, Dmitry Monakhov [off-list ref] wrote:

quoted

O_DIRECT flags can be toggeled via fcntl(F_SETFL).
But this value checked twice inside ext4_file_write_iter() and __generic_file_write()
which result in BUG_ON (see typical stack trace below)
In order to fix this we have to use our own copy of __generic_file_write
and pass o_direct status explicitly.

This seems like a generic problem that would be better served by fixing
the core code instead of making a private copy of such a large function
for ext4.  I expect other filesystems might have similar issues if the
O_DIRECT state is changed in the middle of IO?

One option is to flush pending IO on the file if the O_DIRECT flag is
changed in setfl(). This is a bit heavyweight but I can't imagine any
sane app that is changing the O_DIRECT state on a file repeatedly.

Agree. In fact there are a lot of other issues there fcntl(F_SETFL)
works incorrectly. For example it does not works on stack-fs
(ecrypt,unionfs) if you try to add O_APPEND flags it will be directly
assigned to virtual file (not lower one). For that reason OpenVZ introduced
f_op->set_flags in order to support our stackfs (vefs)
This is reasonable solution in general so I'll prepare patches for mainstream
But this require reasonable time for negotiations with vfs people.

  Agreed. ->set_flags callback looks reasonable.

BTW: same crap with fadvise.

quoted

At the same time currently all versions are affected since 69c499d1/2012-11-29
So we need quick and simple fix for stable releases.
From first glance the best fix is to simply deny toggle O_DIRECT on files.
and f_op->check_flags(new_flags) looks like reasonable candidate, *but*
this interface is 100% useless because it has no access to file_pointer
so we do not know current ->f_flags :)

  [I've CCed linux-fsdevel since it may indeed affect other filesystems]
  I don't think you want to just deny toggling O_DIRECT. That's certainly
going to break some application (however stupid it may be). Also flushing
the IO as Andreas suggests doesn't seem easy because to solve the problem
with changing flags in general, you would have to wait for both reads and
writes, buffered & direct for the struct file and that's not easily doable.
What currently seems as the best solution to me is to store f_flags in the
iocb and then use that for decisions... However it still seems a bit
fragile since people modifying the would would have to have in mind they
have to use iocb->f_flags instead of iocb->ki_filp->f_flags

We can easily serialize AIO,DIO and buf-writers via i_mutex and i_dio_count
the only non-serialized callers are buffered reads and mmaped files.
So tend to agree with iocb->f_flags

								Honza
-- 
Jan Kara [off-list ref]
SUSE Labs, CR
--
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html