On Fri, Oct 04, 2019 at 12:05:54AM +0200, Jan Kara wrote: > diff --git a/fs/ext4/extents.c b/fs/ext4/extents.c > index fb0f99dc8c22..32f2c22c7ef2 100644 > --- a/fs/ext4/extents.c > +++ b/fs/ext4/extents.c > +/* > + * Make sure 'handle' has at least 'check_cred' credits. If not, restart > + * transaction with 'restart_cred' credits. The function drops i_data_sem > + * when restarting transaction and gets it after transaction is restarted. > + * > + * The function returns 0 on success, 1 if transaction had to be restarted, > + * and < 0 in case of fatal error. > + */ > +int ext4_datasem_ensure_credits(handle_t *handle, struct inode *inode, > + int check_cred, int restart_cred) This makes me super nervous. This gets called by ext4_access_path(), which in turn is called by the insert_range, and collapse_range (among others) where we previously were not dropping i_data_sem. This means we will be dropping i_data_sem while they are in the middle of doing surgery to the extent tree, which makes me super nervous. Granted, insert_range and collapse_range take a lot of locks, including the inode lock, but it's not obvious to me that this is safe, and at the very least the documentation for ext4_access_path should have a warning note in its comments that i_data_sem can get dropped, and its call sites audited if they haven't already. Thanks, - Ted
