Re: [PATCH 2/2] sysfs: only access bin file vm_ops with the active lock

[Eric W. Biederman]

Hugh Dickins <hughd@xxxxxxxxxx> writes:

> On Mon, 20 Sep 2010, Eric W. Biederman wrote:
>> 
>> bb->vm_ops is a cached copy of the vm_ops of the underlying
>> sysfs bin file, which means that after sysfs_bin_remove_file
>> completes it is only longer valid to deference bb->vm_ops.
>> 
>> So move all of the tests of bb->vm_ops inside of where
>> we hold the sysfs active lock.
>> 
>> Signed-off-by: Eric W. Biederman <ebiederm@xxxxxxxxxxxxxxxxxx>
>
> Good point.  There's a couple of cases (the ones below) where you've
> changed the return value from success to failure when the entry point
> didn't exist for an object that is no longer active; but on reflection,
> I accept that's just not worth worrying about.

Good spotting and thanks for the review.

Thinking about this it looks like bin_set_policy should only ever
error if the underlying function errors, and that will clear up
one inconsistency.

As for bin_page_mkwrite if sysfs_get_active fails there is no mapping
because we have called unmap_mapping_range to zap everything, so
page_mkwrite should simply not be called.  However there is the
interesting case what should page_mkwrite return if there are no
vm_ops at all.

I expect the right choice for page_mkwrite it to simply have to
different sets of vm_ops and only write of page_mkwrite in one
of them and to simply call VM_FAULT_NOPAGE on error.  Implementing
page_mkwrite when it isn't needed appears to have all kinds of
interesting side effects where we do a lot more work than we other
wise would.  Lock dropping and regrabbing, as well as some extra
dirty page balancing logic gets triggered.

I will see if I can cook up a patch to that effect.

Eric
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/