Re: [PATCH 0/4] btrfs: offline dedupe v2

[Mark Fasheh <mfasheh@suse.de>]

On Tue, Jun 11, 2013 at 11:31:41PM +0200, Gabriel de Perthuis wrote:
> Le 11/06/2013 23:04, Mark Fasheh a écrit :
> > On Tue, Jun 11, 2013 at 10:56:59PM +0200, Gabriel de Perthuis wrote:
> >>> What I found however is that neither of these is a great idea ;)
> >>>
> >>> - We want to require that the inode be open for writing so that an
> >>>   unprivileged user can't do things like run dedupe on a performance
> >>>   sensitive file that they might only have read access to.  In addition I
> >>>   could see it as kind of a surprise (non-standard behavior) to an
> >>>   administrator that users could alter the layout of files they are only
> >>>   allowed to read.
> >>>
> >>> - Readonly snapshots won't let you open for write anyway (unsuprisingly,
> >>>   open() returns -EROFS).  So that kind of kills the idea of them being able
> >>>   to open those files for write which we want to dedupe.
> >>>
> >>> That said, I still think being able to run this against a set of readonly
> >>> snapshots makes sense especially if those snapshots are taken for backup
> >>> purposes. I'm just not sure how we can sanely enable it.
> >>
> >> The check could be: if (fmode_write || cap_sys_admin).
> >>
> >> This isn't incompatible with mnt_want_write, that check is at the
> >> level of the superblocks and vfsmount and not the subvolume fsid.
> > 
> > Oh ok that's certainly better. I think we still have a problem though - how
> > does a process gets write access to a file from a ro-snapshot? If I open a
> > file (as root) on a ro-snapshot on my test machine here I'll get -EROFS.
> 
> Your first series did work in that case.
> The process does get a read-only fd, but that's no obstacle for the ioctl.

Ahh, I had ignored the '||' in your check above. Basically though you have
to have write access unless you're the sysadmin, in which case open for
reading is enough. Makes sense -- I'll try this.
	--Mark

--
Mark Fasheh