Re: [PATCH 3/7] ext4: serialize dio nolocked reads with defrag workers V2

[Jan Kara <jack@suse.cz>]

On Mon 10-09-12 11:31:26, Jan Kara wrote:
> On Sun 09-09-12 21:27:10, Dmitry Monakhov wrote:
> > Inode's block defrag and ext4_change_inode_journal_flag() may
> > affect nonlocked DIO reads result, so proper synchronization
> > required.
> > 
> > - Add missed inode_dio_wait() calls where appropriate
> > - Check inode state under extra i_dio_count reference.
> > 
> > Changes from V1:
> > - add missed memory bariers
> > - move DIOREAD_LOCK state check out from generic should_dioread_nolock
> >   otherwise it will affect existing DIO readers.
>   Looks good. Just maybe I would rename ext4_inode_dio_wait() to
> ext4_inode_block_unlocked_dio(). Anyway you can add:
> Reviewed-by: Jan Kara <jack@suse.cz>
  Oh, I found one more thing:

> > +/* Wait for existing dio workers
> > + * Disable DIO read nolock optimization, so new dioreaders will
> > + * be forced to grab i_mutex
> > +*/
> > +void ext4_inode_dio_wait(struct inode *inode, int resume_readers)
> > +{
> > +	ext4_set_inode_state(inode, EXT4_STATE_DIOREAD_LOCK);
> > +	smp_mb();
> > +	inode_dio_wait(inode);
> > +	if (resume_readers)
> > +		ext4_set_inode_state(inode, EXT4_STATE_DIOREAD_LOCK);
> > +}
> > +
 I've just now noticed the resume_readers argument.
a) You probably meant to call ext4_clear_inode_state() when that argument
is set?
b) You need a smp_mb() before clearing DIOREAD_LOCK?
c) Please remove the resume_readers argument and make a separate function
from that. That new function could be called:
ext4_inode_unblock_unlocked_dio().

								Honza

> >  /*
> >   * O_DIRECT for ext3 (or indirect map) based files
> >   *
> > @@ -810,10 +823,20 @@ retry:
> >  		if (unlikely(!list_empty(&ei->i_completed_io_list)))
> >  			ext4_flush_completed_IO(inode);
> >  
> > +		/* Nolock dioread optimization may be dynamically disabled.
> > +		 * Check inode's state while holding extra i_dio_count ref. */
> > +		atomic_inc(&inode->i_dio_count);
> > +		smp_mb();
> > +		if (!unlikely(ext4_test_inode_state(inode,
> > +						    EXT4_STATE_DIOREAD_LOCK))) {
> > +			inode_dio_done(inode);
> > +			goto retry;
> > +		}
> >  		ret = __blockdev_direct_IO(rw, iocb, inode,
> >  				 inode->i_sb->s_bdev, iov,
> >  				 offset, nr_segs,
> >  				 ext4_get_block, NULL, NULL, 0);
> > +		inode_dio_done(inode);
> >  	} else {
> >  		ret = blockdev_direct_IO(rw, iocb, inode, iov,
> >  				 offset, nr_segs, ext4_get_block);
> > diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
> > index 762b955..ffb4a27 100644
> > --- a/fs/ext4/inode.c
> > +++ b/fs/ext4/inode.c
> > @@ -4720,6 +4720,9 @@ int ext4_change_inode_journal_flag(struct inode *inode, int val)
> >  			return err;
> >  	}
> >  
> > +	/* Wait for all existing dio workers */
> > +	ext4_inode_dio_wait(inode, 0);
> > +
> >  	jbd2_journal_lock_updates(journal);
> >  
> >  	/*
> > @@ -4739,6 +4742,7 @@ int ext4_change_inode_journal_flag(struct inode *inode, int val)
> >  	ext4_set_aops(inode);
> >  
> >  	jbd2_journal_unlock_updates(journal);
> > +	ext4_clear_inode_state(inode, EXT4_STATE_DIOREAD_LOCK);
> >  
> >  	/* Finally we can mark the inode as dirty. */
> >  
> > diff --git a/fs/ext4/move_extent.c b/fs/ext4/move_extent.c
> > index c5826c6..aea0e7a 100644
> > --- a/fs/ext4/move_extent.c
> > +++ b/fs/ext4/move_extent.c
> > @@ -1214,6 +1214,9 @@ ext4_move_extents(struct file *o_filp, struct file *d_filp,
> >  	if (ret1 < 0)
> >  		return ret1;
> >  
> > +	/* Wait for all existing dio workers */
> > +	ext4_inode_dio_wait(orig_inode, 0);
> > +	ext4_inode_dio_wait(donor_inode, 0);
> >  	/* Protect extent tree against block allocations via delalloc */
> >  	double_down_write_data_sem(orig_inode, donor_inode);
> >  	/* Check the filesystem environment whether move_extent can be done */
> > @@ -1412,6 +1415,8 @@ out:
> >  		kfree(holecheck_path);
> >  	}
> >  	double_up_write_data_sem(orig_inode, donor_inode);
> > +	ext4_clear_inode_state(orig_inode, EXT4_STATE_DIOREAD_LOCK);
> > +	ext4_clear_inode_state(donor_inode, EXT4_STATE_DIOREAD_LOCK);
> >  	ret2 = mext_inode_double_unlock(orig_inode, donor_inode);
> >  
> >  	if (ret1)
> > -- 
> > 1.7.7.6
> > 
> -- 
> Jan Kara <jack@suse.cz>
> SUSE Labs, CR
-- 
Jan Kara <jack@suse.cz>
SUSE Labs, CR