>
> > In the general case I don't think that correctly protects against:
> >
> > if (a_ops->call)
> > a_ops->call();
> >
> > Because not all operations are defined in both ext4_aops and
> > ext4_journalled_aops. Specifically migratepage.
> >
> > move_to_new_page() specifically follows the pattern above with migratepage. So
> > is there a bug here?
>
> Looks like there could be.
Ok I'm going to leave this alone and whatever I come up with try and make sure
that the ext4 journal a_ops fits.
[snip]
> >
> > However I have been looking at SRCU because we also have patterns like:
> >
> >
> > generic_file_buffered_read
> > if (a_ops->is_partially_uptodate)
> > a_ops->is_partially_uptodate()
> > page_cache_sync_readahead
> > force_page_cache_readahead
> > if (!a_ops->readpage && !a_ops->readpages)
> > return;
> > __do_page_cache_readahead
> > read_pages
> > if (a_ops->readpages)
> > a_ops->readpages()
> > a_ops->readpage
> >
> >
> > So we would have to pass the a_ops through to use a rwsem. Where SRCU I
> > think would be fine to just take the SRCU read lock multiple times. Am I
> > wrong?
>
> So the idea I had would not solve this issue because we'd release the rwsem
> once we return from ->is_partially_uptodate(). This example shows that we
> actually expect consistency among different aops as they are called in
> sequence and that's much more difficult to achieve than just a consistency
> within single aop call.
I can't be sure but is seems like consistency for an operation is somewhat
important. OR we have to develop rules around when filesystems can, or can not
change a_ops so that the consistency does not matter.
I think what scares me the most is that I'm not really sure what those rules
are...
>
> > We also have a 3rd (2nd?) issue. There are callers who check for the
> > presence of an operation to be used later. For example do_dentry_open():
> >
> > do_dentry_open()
> > {
> > ...
> > if (<flags> & O_DIRECT)
> > if (!<a_ops> || !<a_ops>->direct_IO)
> > return -EINVAL;
> > ...
> > }
> >
> > After this open direct_IO better be there AFAICT so changing the a_ops
> > later would not be good. For ext4 direct_IO is defined for all the
> > a_ops... so I guess that is not a big deal. However, is the user really
> > getting the behavior they expect in this case?
>
> In this particular case I don't think there's any practical harm for any
> filesystem but in general this is another instance where consistency of
> aops over time is assumed.
Yes... But this is just a more complex situation to maintain consistency.
Again I don't have any idea if consistency is required.
But the current definition/use of a_ops is very static (with the exception of
the ext4 case you gave), so I'm thinking that much of the code is written with
the assumption that the vectors do not change.
>
> > I'm afraid of requiring FSs to have to follow rules in defining their a_ops.
> > Because I'm afraid maintaining those rules would be hard and would eventually
> > lead to crashes when someone did it wrong.
>
> I guess this very much depends on the rules. But yes, anything non-obvious
> or hard to check would quickly lead to bugs, I agree. But IMHO fully
> general solution to above problems would clutter the generic code in rather
> ugly way as well because usage of aops is pretty widespread in mm and fs
> code. It isn't just a few places that call them...
I agree it does clutter the code. and the patches I have are not pretty and
I'm not even sure they are not broken... So yea I'm open to ideas!
>
> But I think we could significantly reduce the problem by looking at what's
> in aops. We have lots of operations there that operate on pages. If we
> mandate that before and during switching of aops, you must make sure
> there's nothing in page cache for the inode, you've already dealt with 90%
> of the problems.
Sounds promising...
But digging into this it looks like we need a similar rule for the DAX side to
have no mappings outstanding. Perhaps you meant that when you said page cache?
>
> Beside these we have:
> * write_begin - that creates page in page cache so above rule should stop
> it as well
Just to make sure I understand, do you propose that we put a check in
pagecache_write_[begin|end]() to protect from these calls while changing?
I just want to be sure because I've wondered if we can get away with minimal
checks, or checks on individual functions, in the generic code. But that
seemed kind of ugly as well.
> * bmap - honestly I'd be inclined to just move this to inode_operations
> just like fiemap. There's nothing about address_space in its functionality.
Hmmm... What about these call paths?
ext4_bmap()
filemap_write_and_wait()
filemap_fdatawrite()
__filemap_fdatawrite()
__filemap_fdatawrite_range()
do_writepages()
a_ops->writepages()
or
xfs_vm_bmap()
iomap_bmap()
filemap_write_and_wait()
...
:-/
maybe we should leave it and have it covered under the page cache rule you
propose?
> * swap_activate / swap_deactivate - Either I'd move these to
> file_operations (what's there about address_space, right), or since all
> instances of this only care about the inode, we can as well just pass
> only inode to the function and move it to inode_operations.
XFS calls iomap_swapfile_activate() which calls vfs_fsync() which needs the
file. Seems like file_operations would be better.
I like the idea of cleaning this up a lot. I've gone ahead with a couple of
patches to do this. At least this simplifies things a little bit...
>
> And then the really problematic ones:
> * direct_IO - Logically with how the IO path is structured, it belongs in
> aops so I wouldn't move it. With the advance of iomap it is on its way to
> being removed altogether but that will take a long time to happen
> completely. So for now I'd mandate that direct_IO path must be locked out
> while switching aops.
How do we lock this out between checking for this support on open and using it
later?
I think if we go down this path we have to make a rule that says that direct_IO
must be defined for both a_ops. Right now for our 2 use case we are lucky that
direct_IO is also defined as the same function. So there is little danger of
odd behavior.
Let me explore more.
> * readpages - these should be locked out by the rule that page creation is
> forbidden.
Agreed. Same question applies here as for pagecache_write_[begin|end]().
Should I special case a check for this?
> * writepages - these need to be locked out when switching aops.
If nothing is in the pagecache could we ignore this as well?
>
> And that should be it. So I don't think there's a need for reference-counting
> of aops in the generic code, especially since I don't think it can be done
> in an elegant way (but feel free to correct me). I think that just
> providing a way to lock-out above three calls would be enough.
Ok, I've been thinking about this whole email more today. And what if we add a
couple of FS specific lockout callbacks in struct address_space itself.
If they are defined the FS has dynamic a_ops capability and these 3 functions
will need to be locked out by a rw_sem controlled by the FS.
We can then document the "rules" for dynamic a_ops better for FS's to support
them by referencing the special cases and the fact that dynamic a_ops requires
these callbacks to be defined.
It clutters the generic code a bit, but not as much as my idea. At the same
time it helps to self document the special cases in both the FS's and the core
code.
[snip]
> >
> >
> > I'm still working out the details of using SRCU and a ref count. I have made
> > at least 1 complete pass of all the a_ops users and I think this would cover
> > them all.
>
> Well, my concern with the use of interface like this is:
>
> a) The clutter in the generic code
There is a bit of clutter. What I'm most concerned about is the amount of
special casing. I still think it would be cleaner in the long run... And
force some structure on the use of a_ops. But after looking at it more there
may be a middle ground.
> b) It's difficult to make this work with SRCU because presumably you want
> to use synchronize_srcu() while switching aops. But then you have three
> operations to do:
> 1) switch aops
> 2) set inode flag
> 3) synchronize_srcu
>
> and depending on the order in which you do these either "old aops"
> operations will see inode with a flag or "new aops" will see the inode
> without a flag and either can confuse those functions...
Yes that might be a challenge.
Ira
