On Tue, Apr 02, 2019 at 04:45:23PM +0300, Nikolay Borisov wrote: > > > On 2.04.19 г. 13:07 ч., Anand Jain wrote: > > When the property fails to pass the prop_handlers::validate() check, the > > thread should exit with no changes in the kernel, but as we are starting > > the transaction too early, we have just updated the generation even if > > there is no change. > > > > For example: > > btrfs prop get /btrfs compression > > compression=lzo > > sync > > btrfs in dump-super /dev/sdb | grep "^generation" > > generation 32 > > > > Try to set an incomplete compression type > > > > btrfs prop set /btrfs compression zli > > ERROR: failed to set compression for /btrfs: Invalid argument > > sync > > > > Set failed but generation is incremented > > btrfs in dump-super /dev/sdb | grep "^generation" > > generation 33 <-- > > > > Fix it by collapsing btrfs_set_prop_trans() into btrfs_set_prop(), which > > provides flexibility to start the transaction after the > > prop_handlers::validate() has been checked. > > > > As of now if the prop_handlers::apply() fails then we still increment > > the generation, but if there is strong prop_handlers::validate() then > > the prop_handlers::apply() can never fail. > > > > Signed-off-by: Anand Jain <anand.j...@oracle.com> > > SO what do we gain by this patch? Whether the transaction generation > number (actually in your changelog it will be an improvement to mention > transaction generation explicitly, because there is also inode > generation...) is incremented or not is an implementation detail.
No, the problem is that the order of the preliminary checks and the transaction start is reversed. So first the transaction, then do sanity checks, that are otherwise don't depend on the transaction. Many other places have been updatd to do the lighweight checks first and keep the transaction region minimal.
