Re: [PATCH v2 1/1] ftruncate, truncate: create fanotify events

[Jan Kara]

On Tue 11-11-14 20:55:26, Heinrich Schuchardt wrote:
> On 11.11.2014 12:09, Jan Kara wrote:
> >On Mon 10-11-14 23:34:15, Christoph Hellwig wrote:
> >>On Mon, Nov 10, 2014 at 09:30:29PM +0100, Jan Kara wrote:
> >>>   So what I somewhat dislike about this patch is that notify_change() is
> >>>sometimes called with dentry and sometimes with path. That way it's not
> >>>completely clear when fanotify events will be generated and when not.
> 
> No. With my patch notify_change is still only called with dentry.
> It is only new function notify_change_path which will be called with a path.
> And this function will only be called from do_truncate up to now.
> 
> >>>Sadly it isn't easy to provide struct path in all the places where we are
> >>>calling notify_change() so I'm not sure what would a better solution look
> >>>like either :(
> 
> We only want to create FAN_MODIFY events for ATTR_SIZE. So only for
> these events we need a path.
> 
> To my knowledge notify_change is called with ATTR_SIZE from
> do_truncate(), ecryptfs_truncate() and will be called with ATTR_SIZE
> from ovl_setattr() for a truncation.
  There's also a call in fs/cachefiles/interface.c: cachefiles_attr_changed()
and fs/hpfs/namei.c: hpfs_unlink() and nfs plays with ATTR_SIZE although I
wasn't able to track down whether it actually passes it to notify_change().

So I agree you have covered almost all the cases. But it's just prone to
errors (as you can see when you missed some cases) because it isn't clear
to callers of notify_change() whether and why they should provide the path
via notify_change_path() or not. But see below.

> ecryptfs_dentry_to_lower_path() could be used in ecryptfs_truncate()
> to obtain a path.
> ovl_path_upper() could be used in ovl_setattr() to obtain a path.
  Agreed, this can and should be done.

> >>I suspect the right thing to do is to split out the truncate path
> >>from notify_change, as it's fairly different anyway.
> >   Yeah, that would make sense. I wanted to say it's quite a lot of work to
> >change all the filesystems (where the separation of truncate path makes
> >sense as well IMHO) but actually it's possible to just do the separation at
> >the VFS level and still call ->setattr() fs callback for now. Heinrich will
> >you look into this?
> 
> You seem to agree that struct path has to passed to do_truncate()
> and further to the notification layer.
  Yes.

> Currently do_truncate() calls notify_change() which does not accept
> a path argument.
> 
> Here the size change is implemented as an attribute change.
> Furthermore time attributes are changed.
> 
> What do you exactly mean by
> > just do the separation at the virtual file system level
> > and still call ->setattr() fs callback for now.
> 
> Do you want to duplicate the logic of notify_change() to a
> new function notify_truncate() which will at least have to handle
> ATTR_SIZE, ATTR_FORCE, ATTR_KILL_SUID, ATTR_KILL_SGID, ATTR_FILE and
> time attributes?
> 
> And from notify_truncate() call new function fsnotify_truncate()
> with a logic like fsnotify_modify() but accepting a path?
> 
> This would result in a lot of code duplication.
  Yes, I forgot about other changes coupled with truncate and you're right
that duplicating them would do more bad than good. But what we could do
is to just have __notify_change() which takes both path & dentry as you did
it. notify_change() which takes just dentry and verifies ATTR_SIZE is not
set (WARN_ON_ONCE if it is). notify_change_truncate() which takes path.
This still isn't pretty but at least makes errors harder to do.

								Honza

-- 
Jan Kara <jack@xxxxxxx>
SUSE Labs, CR
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/