Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754528AbbBTMP3 (ORCPT ); Fri, 20 Feb 2015 07:15:29 -0500 Received: from cantor2.suse.de ([195.135.220.15]:57207 "EHLO mx2.suse.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754050AbbBTMP2 (ORCPT ); Fri, 20 Feb 2015 07:15:28 -0500 Date: Fri, 20 Feb 2015 13:15:22 +0100 From: Jan Kara To: Alexey Dobriyan Cc: Jan Kara , Andrew Morton , Al Viro , Linux Kernel , linux-fsdevel , swhiteho@redhat.com, cluster-devel@redhat.com Subject: Re: [PATCH] fs: record task name which froze superblock Message-ID: <20150220121522.GC6293@quack.suse.cz> References: <20150214185524.GA16579@p183.telecom.by> <20150216093852.GB4749@quack.suse.cz> <20150218073455.GA1752@p183.telecom.by> <20150218091323.GA4614@quack.suse.cz> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.5.21 (2010-09-15) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 3862 Lines: 85 On Fri 20-02-15 14:42:29, Alexey Dobriyan wrote: > On Wed, Feb 18, 2015 at 12:13 PM, Jan Kara wrote: > >> > > --- a/fs/ioctl.c > >> > > +++ b/fs/ioctl.c > >> > > @@ -518,6 +518,7 @@ static int ioctl_fioasync(unsigned int fd, struct file *filp, > >> > > static int ioctl_fsfreeze(struct file *filp) > >> > > { > >> > > struct super_block *sb = file_inode(filp)->i_sb; > >> > > + int rv; > >> > > > >> > > if (!capable(CAP_SYS_ADMIN)) > >> > > return -EPERM; > >> > > @@ -527,22 +528,31 @@ static int ioctl_fsfreeze(struct file *filp) > >> > > return -EOPNOTSUPP; > >> > > > >> > > /* Freeze */ > >> > > - if (sb->s_op->freeze_super) > >> > > - return sb->s_op->freeze_super(sb); > >> > > - return freeze_super(sb); > >> > > + if (sb->s_op->freeze_super) { > >> > > + rv = sb->s_op->freeze_super(sb); > >> > > + if (rv == 0) > >> > > + get_task_comm(sb->s_writers.freeze_comm, current); > >> > > + } else > >> > > + rv = freeze_super(sb); > >> > > + return rv; > >> > Why don't you just set the name in ioctl_fsfreeze() in both cases? > >> > >> There are users of freeze_super() in GFS2 unless I'm misreading code. > > Yes, there are. The call in fs/gfs2/glops.c is in a call path from > > ->freeze_super() handler for GFS2 so that one is handled in > > ioctl_fsfreeze() anyway. The call in fs/gfs2/sys.c is a way to freeze > > filesystem via sysfs (dunno why GFS2 has to invent its own thing and ioctl > > isn't enough). Steven? So having the logic in ioctl_fsfreeze(), > > freeze_bdev() and freeze_store() in gfs2 seems to be enough. > > Jan, my logic is as follows. > > Recording freezer task name is not filesystem/device specific and > thus should be done in generic code. So no changes in GFS2. > > freeze_super() is generic counterpart to filesystem/device > specific ->freeze_super() hook, look how they are paired. > It should recore freezer task name, so any future user > will not forget to do the same. > > So it's in ioctl_fsfreeze(), freeze_bdev() and freeze_super(). Well, but this stores the name in two different levels - once in the top level (ioctl_fsfreeze(), freeze_bdev()) and once in a place called from there (freeze_super()). That just seems wrong to me. You can just record the frozen process in freeze_super() and mandate that if someone manages to freeze the fs without calling freeze_super() from his ->freeze_super() handler (currently there isn't such filesystem), he is also responsible for setting freezer name... Hmm? > >> > > --- a/include/linux/fs.h > >> > > +++ b/include/linux/fs.h > >> > > @@ -1221,6 +1221,8 @@ struct sb_writers { > >> > > int frozen; /* Is sb frozen? */ > >> > > wait_queue_head_t wait_unfrozen; /* queue for waiting for > >> > > sb to be thawed */ > >> > > + /* who froze superblock */ > >> > > + char freeze_comm[16]; > >> > Here should be TASK_COMM_LEN, shouldn't it? > >> > >> It will pull sched.h, dunno if we care about headers anymore. > > That's not ideal but IMHO better than having the value hardcoded here. > > That is pretty fragile - i.e. think what happens when someone decides to > > increase TASK_COMM_LEN... > > TASK_COMM_LEN is userspace ABI via at least prctl(PR_SET_NAME). > I can formally move it to include/uapi/linux/sched.h. > This allows to not drag sched.h into fs.h for one tiny define. OK, moving the definition would be preferable to me (and IMO also a cleanup). Honza -- Jan Kara SUSE Labs, CR -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/