From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S932228AbbBBPsA (ORCPT ); Mon, 2 Feb 2015 10:48:00 -0500 Received: from cdptpa-outbound-snat.email.rr.com ([107.14.166.225]:14821 "EHLO cdptpa-oedge-vip.email.rr.com" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S932084AbbBBPrz (ORCPT ); Mon, 2 Feb 2015 10:47:55 -0500 Message-Id: <20150202154703.794777128@goodmis.org> User-Agent: quilt/0.61-1 Date: Mon, 02 Feb 2015 10:47:03 -0500 From: Steven Rostedt To: linux-kernel@vger.kernel.org Cc: Al Viro , Greg Kroah-Hartman , Ingo Molnar , Andrew Morton Subject: [PATCH 0/5 v4] tracing: Add new file system tracefs X-RR-Connecting-IP: 107.14.168.142:25 X-Cloudmark-Score: 0 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Al, Seems we never came up with a solution to how to have mkdir create new files without locking issues. I would like to keep the file creation interface separate from the rest of the tracing code and not have the two coupled tightly. That is, I do not plan on replacing locks like trace_type_lock with dentry->d_inode->i_lock, as I find that is not very robust. Also, you never explained to me why releasing the inode mutex in the tracefs mkdir (and rmdir) functions would brake anything. Yes, there's a possibility that you could have two users, where one user is opening a file, and another user deletes the instance and creates a new instance and the user ends up opening the new instance instead. But as instances and even accessing them should be done by privilege users only, this would be a strange case, especially if one privilege user is deleting an instance that another privilege user is accessing. I'll explain here so that others understand what locking issue we have. When mkdir (and rmdir) handlers are called, the parent dentry inode mutex is held. The mkdir in the instances directory will create more files and directories that have a copy of the top level tracing directory (options, trace files, events, etc). To create an instance, we also need to hold the trace_types_lock, as that is held to iterate all instances to make sure one isn't deleted while being accessed. Thus, for mkdir in an instance director, the lock order will be i_mutex followed by trace_types_lock. But then later, if a module is loaded and we need to create directories for the events that are in the new module, we need to add these directories to each trace array, which means we need to take the trace_arrays_lock to prevent any from being removed, and then create the files. Making the locking order trace_types_lock followed by i_mutex. We have the same problem with event_mutex, which is held to protect the creation of events. In reality, there's no deadlock in mkdir because the only lock that is held when we need to take the trace_arrays_lock or event_mutex is the instance dir lock. And that lock is never taken while holding the trace_arrays_lock or event_mutex. Although, in rmdir, the instance directory lock is held and that is taken when we create a directory, so we need to find a way not to need to hold the trace_arrays_lock when creating the instance instance. directory, which could be done. The trick is, we need to find a way to teach lockdep that there's no deadlock (and make sure there really isn't one). Honestly, unless I know of a real problem with releasing the parent lock (and the instance lock on rmdir), I don't want to go this route. Because all access to files within an instance must first grab the trace array within that instance, which checks that it still exists (under the trace_arrays_lock), and ups its ref count which prevents it from being deleted. Note, the current implementation also releases the lock, and so far there has not been any issues with this. I have two tests that stress the creation, deletion and accessing of the intances. Several threads are created, where some delete instances, other add them, and others read from them. I've had these tests since I created instances and they have found bugs, but nothing yet to do with the releasing of the parent lock. I would like to get these patches into 3.20. As they still do the locking like it has been, but at least now it's not to generic debugfs and is contained in the tracefs inode.c file, I still think this is a bit better. It also solves the mounting of tracing without debugfs. Anyway, back to the description of this patch series: ======================= There has been complaints that tracing is tied too much to debugfs, as there are systems that would like to perform tracing, but do not mount debugfs for security reasons. That is because any subsystem may use debugfs for debugging, and these interfaces are not always tested for security. Creating a new tracefs that the tracing directory will now be attached to allows system admins the ability to access the tracing directory without the need to mount debugfs. Another advantage is that debugfs does not support the system calls for mkdir and rmdir. Tracing uses these system calls to create new instances for sub buffers. This was done by a hack that hijacked the dentry ops from the "instances" debugfs dentry, and replacing it with one that could work. Instead of using this hack, tracefs can provide a proper interface to allow the tracing system to have a mkdir and rmdir feature. To maintain backward compatibility with older tools that expect that the tracing directory is mounted with debugfs, the tracing directory is still created under debugfs and tracefs is automatically mounted there. Finally, a new directory is created when tracefs is enabled called /sys/kernel/tracing. This will be the new location that system admins may mount tracefs if they are not using debugfs. Changes from v3: o Instead of pulling in Al's patches, I just pulled in his tree and placed thes patches on top of it (thus, Al's patches are not in this series). o Change tracefs to reflect the same changes that Al did to debugfs. That is, I based tracefs on top of debugfs, these patches show that I did it on top of debugfs + Al's changes to them. o Added a new method called tracefs_create_instance_dir(). This removes needing to use the dentry->d_inode->i_private, and only allows for one mkdir/rmdir ops to be set. This is then saved by the tracefs system and the mkdir/rmdir can only be used by the instance dir that is returned. Changes from v2: o Al Viro wrote a series of patches to create "debugfs_create_automount()" that gives us a safe way to modify the debugfs dentry to add automounting. tracefs now uses this function to automount itself on the debugfs/tracing directory. Changes from v1: o Fixed all the posting problems (included files that were missing and removed changes that were not suppose to be there). o Changed the mkdir/rmdir logic. Instead of trying to keep the inode mutexes locked, which caused locking issues with other locks that were taken, the locks are still released. But this time, they are released by the tracefs system which has a bit more control. As the mkdir/rmdir methods for the tracing facility only need the new name of the instance, the tracefs mkdir/rmdir copies the name from the dentry, releases the locks, and passes in the copy to the tracing methods. Internal SHA1: fbd7add127fedbb79cccc2890241bb6c1b195f69 Steven Rostedt (Red Hat) (5): tracefs: Add new tracefs file system tracing: Convert the tracing facility over to use tracefs tracing: Automatically mount tracefs on debugfs/tracing tracefs: Add directory /sys/kernel/tracing tracing: Have mkdir and rmdir be part of tracefs ---- fs/Makefile | 1 + fs/tracefs/Makefile | 4 + fs/tracefs/inode.c | 650 +++++++++++++++++++++++++++++++++++ include/linux/tracefs.h | 45 +++ include/uapi/linux/magic.h | 2 + kernel/trace/ftrace.c | 22 +- kernel/trace/trace.c | 160 ++++----- kernel/trace/trace.h | 2 +- kernel/trace/trace_events.c | 32 +- kernel/trace/trace_functions_graph.c | 7 +- kernel/trace/trace_kprobe.c | 10 +- kernel/trace/trace_probe.h | 2 +- kernel/trace/trace_stat.c | 10 +- 13 files changed, 811 insertions(+), 136 deletions(-)