No subject

false: do sb_start_write(SB_FREEZE_TRANS) in trans alloc (will call sb_stop_write in trans free)
true: skip sb_start_write & set flag to skip sb_stop_write(SB_FREEZE_TRANS) in trans free

so maybe:

#define XFS_HONOR_FREEZE_TRANS	false
#define XFS_IGNORE_FREEZE_TRANS	true

would be a little clearer?  Or maybe:

#define XFS_INC_FREEZE_TRANS	false
#define XFS_NOINC_FREEZE_TRANS	true

or

#define XFS_TRANS_START_WRITE		false
#define XFS_NO_TRANS_START_WRITE	true

bleah, ok, step away from the bike shed....

> as the parameters here to makeit extremely clear when reading the
> code exactly what that last parameter means. i.e. it is self
> documenting. That will help clear up a lot of the confusion on what
> these magic boolean parameters are supposed to mean....
> 
>> @@ -312,7 +311,6 @@ void xfs_do_force_shutdown(struct xfs_mount *mp, int flags, char *fname,
>>  #define SHUTDOWN_DEVICE_REQ	0x0020	/* failed all paths to the device */
>>  
>>  #define xfs_test_for_freeze(mp)		((mp)->m_super->s_frozen)
> 
> I'd remove this, too, and just open code it.
> 
>> diff --git a/fs/xfs/xfs_sync.c b/fs/xfs/xfs_sync.c
>> index aa3dc1a..24f4d7c 100644
>> --- a/fs/xfs/xfs_sync.c
>> +++ b/fs/xfs/xfs_sync.c
>> @@ -373,7 +373,7 @@ xfs_quiesce_data(
>>  
>>  	/* mark the log as covered if needed */
>>  	if (xfs_log_need_covered(mp))
>> -		error2 = xfs_fs_log_dummy(mp);
>> +		error2 = xfs_fs_log_dummy(mp, false);
> 
> This is the call that can occur inside SB_FREEZE_WRITE context as
> well as outside it.

Somehow I'm missing the problem here.  This basically means that we will
always increment the metadata writer count for the new transaction, and drop
it when done.  But I _think_ that's ok in both spots, no?  Neither is called
inside of a FREEZE_TRANS.

-Eric


>>  
>>  	/* flush data-only devices */
>>  	if (mp->m_rtdev_targp)
>> @@ -421,18 +421,11 @@ xfs_quiesce_attr(
>>  	int	error = 0;
>>  
>>  	/* wait for all modifications to complete */
>> -	while (atomic_read(&mp->m_active_trans) > 0)
>> -		delay(100);
>> +	sb_wait_write(mp->m_super, SB_FREEZE_TRANS);
>>  
>>  	/* flush inodes and push all remaining buffers out to disk */
>>  	xfs_quiesce_fs(mp);
>>  
>> -	/*
>> -	 * Just warn here till VFS can correctly support
>> -	 * read-only remount without racing.
>> -	 */
>> -	WARN_ON(atomic_read(&mp->m_active_trans) != 0);
>> -
> 
> Now there's an interesting question. Does this break read-only
> remount?
> 
> /me checks the sb_wait_write() code
> 
> No, it looks like it should be fine.
> 
>>  	/* Push the superblock and write an unmount record */
>>  	error = xfs_log_sbcount(mp);
>>  	if (error)
>> @@ -467,7 +460,7 @@ xfs_sync_worker(
>>  		/* dgc: errors ignored here */
>>  		if (mp->m_super->s_frozen == SB_UNFROZEN &&
>>  		    xfs_log_need_covered(mp))
>> -			error = xfs_fs_log_dummy(mp);
>> +			error = xfs_fs_log_dummy(mp, false);
>>  		else
>>  			xfs_log_force(mp, 0);
>>  		error = xfs_qm_sync(mp, SYNC_TRYLOCK);
>> diff --git a/fs/xfs/xfs_trans.c b/fs/xfs/xfs_trans.c
>> index 1f35b2f..e97014b 100644
>> --- a/fs/xfs/xfs_trans.c
>> +++ b/fs/xfs/xfs_trans.c
>> @@ -577,24 +577,28 @@ xfs_trans_alloc(
>>  	xfs_mount_t	*mp,
>>  	uint		type)
>>  {
>> -	xfs_wait_for_freeze(mp, SB_FREEZE_TRANS);
>> -	return _xfs_trans_alloc(mp, type, KM_SLEEP);
>> +	return _xfs_trans_alloc(mp, type, KM_SLEEP, false);
>>  }
>>  
>>  xfs_trans_t *
>>  _xfs_trans_alloc(
>>  	xfs_mount_t	*mp,
>>  	uint		type,
>> -	uint		memflags)
>> +	uint		memflags,
>> +	bool		freezing)
>>  {
>>  	xfs_trans_t	*tp;
>>  
>> -	atomic_inc(&mp->m_active_trans);
>> -
>> +	if (!freezing)
>> +		sb_start_write(mp->m_super, SB_FREEZE_TRANS);
>> +	else
>> +		WARN_ON(xfs_test_for_freeze(mp) != SB_FREEZE_TRANS);
> 
> Just open code xfs_test_for_freeze() and add a line of whitespace
> after this.
> 
>>  	tp = kmem_zone_zalloc(xfs_trans_zone, memflags);
>>  	tp->t_magic = XFS_TRANS_MAGIC;
>>  	tp->t_type = type;
>>  	tp->t_mountp = mp;
>> +	if (freezing)
>> +		tp->t_flags |= XFS_TRANS_FREEZING;
> 
> Simply assign the value - tp->t_flags is guaranteed to be 0 right
> now.
> 
>>  	INIT_LIST_HEAD(&tp->t_items);
>>  	INIT_LIST_HEAD(&tp->t_busy);
>>  	return tp;
>> @@ -611,7 +615,8 @@ xfs_trans_free(
>>  	xfs_alloc_busy_sort(&tp->t_busy);
>>  	xfs_alloc_busy_clear(tp->t_mountp, &tp->t_busy, false);
>>  
>> -	atomic_dec(&tp->t_mountp->m_active_trans);
>> +	if (!(tp->t_flags & XFS_TRANS_FREEZING))
>> +		sb_end_write(tp->t_mountp->m_super, SB_FREEZE_TRANS);
>>  	xfs_trans_free_dqinfo(tp);
>>  	kmem_zone_free(xfs_trans_zone, tp);
>>  }
>> @@ -654,7 +659,7 @@ xfs_trans_dup(
>>  
>>  	xfs_trans_dup_dqinfo(tp, ntp);
>>  
>> -	atomic_inc(&tp->t_mountp->m_active_trans);
>> +	sb_dup_write(tp->t_mountp->m_super, SB_FREEZE_TRANS);
> 
> That's strangly named. Isn't the normal thing to do here use a "__"
> prefix for operations that just need an extra reference because they
> already have one (i.e. __sb_start_write())?
> 
> This also looks broken with repsect to the new XFS_TRANS_FREEZING
> flag. If it is set on the parent, it needs to be set on the
> duplicated transaction. And if it is set, then no extra reference
> should be taken.
> 
> Cheers,
> 
> Dave.