Re: [PATCH] quota: Lock s_umount in exclusive mode for Q_XQUOTA{ON,OFF} quotactls.

[Javier Barrio]

On 20/12/18 9:20, Jan Kara wrote:
> On Fri 14-12-18 10:56:13, Jan Kara wrote:
>> On Thu 13-12-18 01:06:29, Javier Barrio wrote:
>>> Commit 1fa5efe3622db58cb8c7b9a50665e9eb9a6c7e97 (ext4: Use generic
>>> helpers for quotaon and quotaoff) made possible to call
>>> quotactl(Q_XQUOTAON/OFF) on ext4 filesystems with sysfile quota support.
>>> This leads to calling dquot_enable/disable without s_umount held in excl.
>>> mode, because quotactl_cmd_onoff checks only for Q_QUOTAON/OFF.
>>>
>>> The following WARN_ON_ONCE triggers (in this case for dquot_enable, ext4,
>>> latest Linus' tree):
>>>
>>> [  155.036847] WARNING: CPU: 0 PID: 2343 at fs/quota/dquot.c:2469 dquot_enable+0x34/0xb9
>>
>> ...
>>
>>> This patch locks the superblock ->s_umount sem. in exclusive mode for all Q_XQUOTAON/OFF
>>> quotactls too in addition to Q_QUOTAON/OFF.
>>
>> Thanks for the patch! It looks good to me but let me run it past XFS
>> people. Looking at XFS code they definitely do not *need* s_umount in
>> exclusive mode for Q_XQUOTAON/OFF (they have their private mutex for
>> the exclusion). Shared mode they currently get is enough for them. But
>> exclusive mode is fine for them as well AFAICT and it would be easier if
>> all quota backends had the same locking rules wrt VFS locks. XFS guys, any
>> objections to switching Q_XQUOTAON/OFF handlers from having s_umount locked
>> for read to having it locked exclusive?
> 
> Since there were no objections and the patch passed quota xfstests for XFS,
> I've added it to my tree and will push it to Linus in the coming merge
> window.
> 
> 								Honza
> 

Thank you!


	Javier