Re: [PATCH 32/53f] ext4: move dcache modifying code out of __ext4_link()

[Jan Kara]

On Wed 18-03-26 07:27:37, NeilBrown wrote:
> > > @@ -3460,8 +3460,6 @@ int __ext4_link(struct inode *dir, struct inode *inode, struct dentry *dentry)
> > >  		ext4_handle_sync(handle);
> > >  
> > >  	inode_set_ctime_current(inode);
> > > -	ext4_inc_count(inode);
> > > -	ihold(inode);
> > >  
> > >  	err = ext4_add_entry(handle, dentry, inode);
> > >  	if (!err) {
> > > @@ -3471,11 +3469,7 @@ int __ext4_link(struct inode *dir, struct inode *inode, struct dentry *dentry)
> > >  		 */
> > >  		if (inode->i_nlink == 1)
> > >  			ext4_orphan_del(handle, inode);
> > > -		d_instantiate(dentry, inode);
> > > -		ext4_fc_track_link(handle, dentry);
> > > -	} else {
> > > -		drop_nlink(inode);
> > > -		iput(inode);
> > > +		__ext4_fc_track_link(handle, inode, dentry);
> > 
> > This looks wrong. If fastcommit replay is running, we must skip calling
> > __ext4_fc_track_link(). Similarly if the filesystem is currently
> > inelligible for fastcommit (due to some complex unsupported operations
> > running in parallel). Why did you change ext4_fc_track_link() to
> > __ext4_fc_track_link()?
> 
> I changed to __ext4_fc_track_link() because I needed something that
> accepted the inode separately from the dentry.  As you point out, that
> means we lose some important code which makes the decision misguided.
> 
> I'm wondering about taking a different approach - not using a dentry at
> all and not constifying anything.
> I could split __ext4_add_entry() out of ext4_add_entry() and instead of
> passing the dentry-to-add I could pass the dir inode qstr name, and
> d_flags.
> 
> Then ext4_link could be passed the same plus a "do fast commit" flag.
> 
> The result would be more verbose, but also hopefully more clear.

Yeah, I was considering that option as well when looking at this. Let's see
how the code will look like.

								Honza

-- 
Jan Kara <jack@xxxxxxxx>
SUSE Labs, CR