Jan Kara <jack@xxxxxxx> writes: > On Fri 10-10-14 11:48:30, Dmitry Monakhov wrote: >> Andreas Dilger <adilger@xxxxxxxxx> writes: >> >> > On Oct 9, 2014, at 5:14 AM, Dmitry Monakhov <dmonakhov@xxxxxxxxxx> wrote: >> >> 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. > >> 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 <jack@xxxxxxx> > SUSE Labs, CR > -- > To unsubscribe from this list: send the line "unsubscribe linux-ext4" in > the body of a message to majordomo@xxxxxxxxxxxxxxx > More majordomo info at http://vger.kernel.org/majordomo-info.html
Attachment:
pgpzDHO5XtgU0.pgp
Description: PGP signature
