| Message ID | 20180731072928.2413-1-avagin@openvz.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [dhowells/mount-context] fs: don't call fs_context->free() from fsmount() | expand |
Andrei Vagin <avagin@openvz.org> wrote: > @@ -3435,9 +3435,6 @@ SYSCALL_DEFINE3(fsmount, int, fs_fd, unsigned int, flags, unsigned int, ms_flags > * do any memory allocation or anything like that at this point as we > * don't want to have to handle any errors incurred. > */ > - if (fc->ops && fc->ops->free) > - fc->ops->free(fc); > - fc->fs_private = NULL; > fc->s_fs_info = NULL; > fc->sb_flags = 0; > fc->sloppy = false; This isn't the right fix. The context needs to be reset at this point so that it's prepared to be reinitialised into in the same state as one generated by fspick(). I can do this two ways: (1) stick a flag in the context that says if ->free() needs calling, (2) make all the ->free() routines aware that they may see the reset state. I think (1) is less error prone. David
On Tue, Jul 31, 2018 at 09:52:36AM +0100, David Howells wrote: > Andrei Vagin <avagin@openvz.org> wrote: > > > @@ -3435,9 +3435,6 @@ SYSCALL_DEFINE3(fsmount, int, fs_fd, unsigned int, flags, unsigned int, ms_flags > > * do any memory allocation or anything like that at this point as we > > * don't want to have to handle any errors incurred. > > */ > > - if (fc->ops && fc->ops->free) > > - fc->ops->free(fc); > > - fc->fs_private = NULL; > > fc->s_fs_info = NULL; > > fc->sb_flags = 0; > > fc->sloppy = false; > > This isn't the right fix. The context needs to be reset at this point so that > it's prepared to be reinitialised into in the same state as one generated by > fspick(). I understand this. init_fs_context() is called from fspick() and fs_context->free() is called for contexts which have been created in fspick(). > > I can do this two ways: (1) stick a flag in the context that says if ->free() > needs calling, (2) make all the ->free() routines aware that they may see the > reset state. I think (1) is less error prone. Does it mean that fc->fs_type->init_fs_context() should not be called contexts which are created from fspick()? > > David
Andrei Vagin <avagin@virtuozzo.com> wrote: > > I can do this two ways: (1) stick a flag in the context that says if > > ->free() needs calling, (2) make all the ->free() routines aware that they > > may see the reset state. I think (1) is less error prone. > > Does it mean that fc->fs_type->init_fs_context() should not be called > contexts which are created from fspick()? No. I've put a flag in the context that is set when ->init_fs_context() is called and cleared when ->free() is called. ->free() isn't called in the put routine if the flag isn't set. David
On Tue, Jul 31, 2018 at 09:56:45PM +0100, David Howells wrote: > Andrei Vagin <avagin@virtuozzo.com> wrote: > > > > I can do this two ways: (1) stick a flag in the context that says if > > > ->free() needs calling, (2) make all the ->free() routines aware that they > > > may see the reset state. I think (1) is less error prone. > > > > Does it mean that fc->fs_type->init_fs_context() should not be called > > contexts which are created from fspick()? > > No. I've put a flag in the context that is set when ->init_fs_context() is > called and cleared when ->free() is called. ->free() isn't called in the put > routine if the flag isn't set. > /* We've done the mount bit - now move the file context into > * more or less the same state as if we'd done an fspick(). According to this comment, a context after fsmount() should be in the same state as after fspick(). In fsmount(), we call ->free() which is oposite to init_fs_context(). If we want to have "more or less the same state" and want to call fs_context->free() in fsmount(), this means that we should not call fc->fs_type->init_fs_context() in fspick()... Where am I wrong? > > David
Andrei Vagin <avagin@virtuozzo.com> wrote: > > > Does it mean that fc->fs_type->init_fs_context() should not be called > > > contexts which are created from fspick()? > > > > No. I've put a flag in the context that is set when ->init_fs_context() is > > called and cleared when ->free() is called. ->free() isn't called in the put > > routine if the flag isn't set. > > > /* We've done the mount bit - now move the file context into > > * more or less the same state as if we'd done an fspick(). > > According to this comment, a context after fsmount() should be in > the same state as after fspick(). > > In fsmount(), we call ->free() which is oposite to init_fs_context(). If > we want to have "more or less the same state" and want to call > fs_context->free() in fsmount(), this means that we should not call > fc->fs_type->init_fs_context() in fspick()... Where am I wrong? vfs_fsconfig() calls ->init_fs_context() if fc->phase is FS_CONTEXT_AWAITING_RECONF. This is so that we don't actually fully reinit the context unnecessarily if the fd is just going to be closed after fsmount() is called. fspick() assumes that you're definitely going to reconfigure the superblock (presumably that's why you used it) and always reinitialises the context, shoving fc->phase round to FS_CONTEXT_RECONF_PARAMS. David
On Wed, Aug 01, 2018 at 01:42:29AM +0100, David Howells wrote: > Andrei Vagin <avagin@virtuozzo.com> wrote: > > > > > Does it mean that fc->fs_type->init_fs_context() should not be called > > > > contexts which are created from fspick()? > > > > > > No. I've put a flag in the context that is set when ->init_fs_context() is > > > called and cleared when ->free() is called. ->free() isn't called in the put > > > routine if the flag isn't set. > > > > > /* We've done the mount bit - now move the file context into > > > * more or less the same state as if we'd done an fspick(). > > > > According to this comment, a context after fsmount() should be in > > the same state as after fspick(). > > > > In fsmount(), we call ->free() which is oposite to init_fs_context(). If > > we want to have "more or less the same state" and want to call > > fs_context->free() in fsmount(), this means that we should not call > > fc->fs_type->init_fs_context() in fspick()... Where am I wrong? > > vfs_fsconfig() calls ->init_fs_context() if fc->phase is > FS_CONTEXT_AWAITING_RECONF. This is so that we don't actually fully reinit > the context unnecessarily if the fd is just going to be closed after fsmount() > is called. I understood the idea. Thank you for the explanation. > > fspick() assumes that you're definitely going to reconfigure the superblock > (presumably that's why you used it) and always reinitialises the context, > shoving fc->phase round to FS_CONTEXT_RECONF_PARAMS. > > David
diff --git a/fs/namespace.c b/fs/namespace.c index 7e7b1145d15d..1899456194c3 100644 --- a/fs/namespace.c +++ b/fs/namespace.c @@ -3435,9 +3435,6 @@ SYSCALL_DEFINE3(fsmount, int, fs_fd, unsigned int, flags, unsigned int, ms_flags * do any memory allocation or anything like that at this point as we * don't want to have to handle any errors incurred. */ - if (fc->ops && fc->ops->free) - fc->ops->free(fc); - fc->fs_private = NULL; fc->s_fs_info = NULL; fc->sb_flags = 0; fc->sloppy = false;